You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "aglinxinyuan (via GitHub)" <gi...@apache.org> on 2026/07/04 04:29:32 UTC
[PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
aglinxinyuan opened a new pull request, #6083:
URL: https://github.com/apache/texera/pull/6083
### What changes were proposed in this PR?
Add unit test coverage for three `common/config` objects, selected from the Codecov report (all 0%). No production-code changes.
| File | Codecov before | What the tests pin |
| --- | --- | --- |
| `EnvironmentalVariable.scala` | 0% | every env-var-name constant resolves, is non-empty, and is unique; `get` returns `None`/`Some` for unset/set vars |
| `ApplicationConfig.scala` | 0% | every value resolves from application.conf with its default (constants, flow-control, network-buffering, reconfiguration, fault-tolerance, scheduling, cleanup, web-server, ai-assistant) |
| `GuiConfig.scala` | 0% | every gui.conf boolean/string/int setting resolves to its default |
Reading each value forces resolution from the backing `.conf`, so a renamed or mistyped key surfaces here as a `ConfigException` instead of at service start-up. Every key carries a `${?ENV}` override, so exact-value assertions are guarded on the env var being unset — mirroring the existing `StorageConfigSpec`.
### Any related issues, documentation, discussions?
Follow-up to the review feedback on #6043: prioritize tests that fill uncovered code paths.
### How was this PR tested?
- `sbt "Config/testOnly *EnvironmentalVariableSpec *ApplicationConfigSpec *GuiConfigSpec"` — 16 tests, all green
- `sbt "Config/Test/scalafmtCheck"` and `sbt "Config/scalafixAll --check"` — clean
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8 [1M context])
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4881085739
Good call — switched `EnvironmentalVariableSpec` to reflectively enumerate the object's no-arg `ENV_*` String accessors, so a newly added constant is automatically covered by the non-empty and uniqueness checks and the list can't drift out of sync. Thanks for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4880644016
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/6083?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 57.64%. Comparing base ([`93f4b68`](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`ac3b646`](https://app.codecov.io/gh/apache/texera/commit/ac3b646fc65c9a8953fb723d2b193ab16c4401b6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #6083 +/- ##
=========================================
Coverage 57.64% 57.64%
Complexity 3142 3142
=========================================
Files 1130 1130
Lines 43832 43832
Branches 4747 4747
=========================================
Hits 25268 25268
Misses 17122 17122
Partials 1442 1442
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `44.59% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.69% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `52.30% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `62.81% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.06% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.57% <ø> (ø)` | | |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.15% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [python](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `90.69% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/6083/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `55.14% <ø> (ø)` | | Carriedforward from [93f4b68](https://app.codecov.io/gh/apache/texera/commit/93f4b6838f64550771be527e53038d5214c6981c?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/6083?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on code in PR #6083:
URL: https://github.com/apache/texera/pull/6083#discussion_r3522608685
##########
common/config/src/test/scala/org/apache/texera/common/config/ApplicationConfigSpec.scala:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.texera.common.config
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * Spec for [[ApplicationConfig]]. Reading each value forces resolution from application.conf, so
+ * a renamed or mistyped key surfaces here as a ConfigException instead of at service start-up.
+ * Every key carries a `${?ENV_VAR}` override, so exact-value assertions that could be overridden
+ * are guarded on the env var being unset (mirroring StorageConfigSpec).
+ */
+class ApplicationConfigSpec extends AnyFlatSpec with Matchers {
+
+ "ApplicationConfig constants" should "load the default constant values" in {
+ ApplicationConfig.loggingQueueSizeInterval shouldBe 30000
+ ApplicationConfig.MAX_RESOLUTION_ROWS shouldBe 2000
+ ApplicationConfig.MAX_RESOLUTION_COLUMNS shouldBe 2000
+ ApplicationConfig.numWorkerPerOperatorByDefault shouldBe 2
Review Comment:
Done — added an `ifUnset` helper and wrapped every exact-value assertion; it treats the override as present if set via either the environment or a JVM system property (application.conf falls back to ConfigFactory.load(), which layers system properties).
##########
common/config/src/test/scala/org/apache/texera/common/config/EnvironmentalVariableSpec.scala:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.texera.common.config
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * Spec for [[EnvironmentalVariable]], the registry of environment-variable *names* used across
+ * the *.conf files. Referencing every constant forces the whole object to initialize, so a
+ * deleted or renamed constant surfaces here; asserting distinctness guards against two settings
+ * accidentally sharing one env-var name.
+ */
+class EnvironmentalVariableSpec extends AnyFlatSpec with Matchers {
+
+ import EnvironmentalVariable._
+
+ private val allNames: Seq[String] = Seq(
+ ENV_JAVA_OPTS,
+ ENV_FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT,
+ ENV_FILE_SERVICE_UPLOAD_ONE_FILE_TO_DATASET_ENDPOINT,
+ ENV_USER_JWT_TOKEN,
+ ENV_AUTH_JWT_SECRET,
+ ENV_JDBC_URL,
+ ENV_JDBC_USERNAME,
+ ENV_JDBC_PASSWORD,
+ ENV_ICEBERG_CATALOG_TYPE,
+ ENV_ICEBERG_CATALOG_REST_URI,
+ ENV_ICEBERG_CATALOG_REST_WAREHOUSE_NAME,
+ ENV_ICEBERG_CATALOG_POSTGRES_URI_WITHOUT_SCHEME,
+ ENV_ICEBERG_CATALOG_POSTGRES_USERNAME,
+ ENV_ICEBERG_CATALOG_POSTGRES_PASSWORD,
+ ENV_ICEBERG_TABLE_RESULT_NAMESPACE,
+ ENV_ICEBERG_TABLE_CONSOLE_MESSAGES_NAMESPACE,
+ ENV_ICEBERG_TABLE_RUNTIME_STATISTICS_NAMESPACE,
+ ENV_ICEBERG_TABLE_STATE_NAMESPACE,
+ ENV_ICEBERG_TABLE_COMMIT_BATCH_SIZE,
+ ENV_ICEBERG_TABLE_COMMIT_NUM_RETRIES,
+ ENV_ICEBERG_TABLE_COMMIT_MIN_WAIT_MS,
+ ENV_ICEBERG_TABLE_COMMIT_MAX_WAIT_MS,
+ ENV_LAKEFS_ENDPOINT,
+ ENV_LAKEFS_AUTH_API_SECRET,
+ ENV_LAKEFS_AUTH_USERNAME,
+ ENV_LAKEFS_AUTH_PASSWORD,
+ ENV_LAKEFS_BLOCK_STORAGE_TYPE,
+ ENV_LAKEFS_BLOCK_STORAGE_BUCKET_NAME,
+ ENV_S3_ENDPOINT,
+ ENV_S3_REGION,
+ ENV_S3_AUTH_USERNAME,
+ ENV_S3_AUTH_PASSWORD,
+ ENV_CONSTANTS_LOGGING_QUEUE_SIZE_INTERVAL,
+ ENV_CONSTANTS_NUM_WORKER_PER_OPERATOR,
+ ENV_CONSTANTS_MAX_RESOLUTION_ROWS,
+ ENV_CONSTANTS_MAX_RESOLUTION_COLUMNS,
+ ENV_CONSTANTS_STATUS_UPDATE_INTERVAL,
+ ENV_FLOW_CONTROL_MAX_CREDIT_ALLOWED_IN_BYTES_PER_CHANNEL,
+ ENV_FLOW_CONTROL_CREDIT_POLL_INTERVAL_IN_MS,
+ ENV_NETWORK_BUFFERING_DEFAULT_DATA_TRANSFER_BATCH_SIZE,
+ ENV_NETWORK_BUFFERING_ENABLE_ADAPTIVE_BUFFERING,
+ ENV_NETWORK_BUFFERING_ADAPTIVE_BUFFERING_TIMEOUT_MS,
+ ENV_RECONFIGURATION_ENABLE_TRANSACTIONAL_RECONFIGURATION,
+ ENV_CACHE_ENABLED,
+ ENV_USER_SYS_ENABLED,
+ ENV_USER_SYS_GOOGLE_CLIENT_ID,
+ ENV_USER_SYS_GOOGLE_SMTP_GMAIL,
+ ENV_USER_SYS_GOOGLE_SMTP_PASSWORD,
+ ENV_USER_SYS_VERSION_TIME_LIMIT_IN_MINUTES,
+ ENV_RESULT_CLEANUP_TTL_IN_SECONDS,
+ ENV_RESULT_CLEANUP_COLLECTION_CHECK_INTERVAL_IN_SECONDS,
+ ENV_WEB_SERVER_WORKFLOW_STATE_CLEANUP_IN_SECONDS,
+ ENV_WEB_SERVER_PYTHON_CONSOLE_BUFFER_SIZE,
+ ENV_WEB_SERVER_WORKFLOW_RESULT_PULLING_IN_SECONDS,
+ ENV_WEB_SERVER_CLEAN_ALL_EXECUTION_RESULTS_ON_SERVER_START,
+ ENV_MAX_WORKFLOW_WEBSOCKET_REQUEST_PAYLOAD_SIZE_KB,
+ ENV_FAULT_TOLERANCE_LOG_STORAGE_URI,
+ ENV_FAULT_TOLERANCE_LOG_FLUSH_INTERVAL_MS,
+ ENV_FAULT_TOLERANCE_LOG_RECORD_MAX_SIZE_IN_BYTE,
+ ENV_FAULT_TOLERANCE_MAX_SUPPORTED_RESEND_QUEUE_LENGTH,
+ ENV_FAULT_TOLERANCE_DELAY_BEFORE_RECOVERY,
+ ENV_FAULT_TOLERANCE_HDFS_STORAGE_ADDRESS,
+ ENV_SCHEDULE_GENERATOR_ENABLE_COST_BASED_SCHEDULE_GENERATOR,
+ ENV_SCHEDULE_GENERATOR_MAX_CONCURRENT_REGIONS,
+ ENV_SCHEDULE_GENERATOR_USE_GLOBAL_SEARCH,
+ ENV_SCHEDULE_GENERATOR_USE_TOP_DOWN_SEARCH,
+ ENV_SCHEDULE_GENERATOR_SEARCH_TIMEOUT_MILLISECONDS,
+ ENV_AI_ASSISTANT_SERVER_ASSISTANT,
+ ENV_AI_ASSISTANT_SERVER_AI_SERVICE_KEY,
+ ENV_AI_ASSISTANT_SERVER_AI_SERVICE_URL
+ )
+
+ "EnvironmentalVariable" should "expose non-empty, unique env-var names" in {
+ allNames.foreach(name => name.trim should not be empty)
+ allNames.distinct.size shouldBe allNames.size
+ }
+
+ it should "use the documented names for a representative sample" in {
+ ENV_JAVA_OPTS shouldBe "JAVA_OPTS"
+ ENV_AUTH_JWT_SECRET shouldBe "AUTH_JWT_SECRET"
+ ENV_JDBC_URL shouldBe "STORAGE_JDBC_URL"
+ ENV_S3_ENDPOINT shouldBe "STORAGE_S3_ENDPOINT"
+ ENV_CACHE_ENABLED shouldBe "CACHE_ENABLED"
+ }
+
+ "EnvironmentalVariable.get" should "return None for an unset variable" in {
+ val absent = "TEXERA_DEFINITELY_UNSET_" + System.nanoTime().toString
+ EnvironmentalVariable.get(absent) shouldBe None
+ }
+
+ it should "return Some(value) for a variable that is set in the JVM environment" in {
+ val env = System.getenv()
+ if (!env.isEmpty) {
+ val entry = env.entrySet().iterator().next()
+ EnvironmentalVariable.get(entry.getKey) shouldBe Some(entry.getValue)
+ }
+ }
Review Comment:
Done — the test now `cancel`s when the JVM environment is empty, so it reports as canceled rather than passing without asserting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4880637737
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- No candidates found from `git blame` history.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #6083:
URL: https://github.com/apache/texera/pull/6083
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #6083:
URL: https://github.com/apache/texera/pull/6083#discussion_r3522566673
##########
common/config/src/test/scala/org/apache/texera/common/config/ApplicationConfigSpec.scala:
##########
@@ -0,0 +1,90 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.texera.common.config
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * Spec for [[ApplicationConfig]]. Reading each value forces resolution from application.conf, so
+ * a renamed or mistyped key surfaces here as a ConfigException instead of at service start-up.
+ * Every key carries a `${?ENV_VAR}` override, so exact-value assertions that could be overridden
+ * are guarded on the env var being unset (mirroring StorageConfigSpec).
+ */
+class ApplicationConfigSpec extends AnyFlatSpec with Matchers {
+
+ "ApplicationConfig constants" should "load the default constant values" in {
+ ApplicationConfig.loggingQueueSizeInterval shouldBe 30000
+ ApplicationConfig.MAX_RESOLUTION_ROWS shouldBe 2000
+ ApplicationConfig.MAX_RESOLUTION_COLUMNS shouldBe 2000
+ ApplicationConfig.numWorkerPerOperatorByDefault shouldBe 2
Review Comment:
These assertions assume all `${?...}` env overrides in `application.conf` are unset, but the spec doesn’t guard them (unlike `StorageConfigSpec` / `GuiConfigSpec`). If a developer (or CI) sets any of the corresponding env vars (e.g. `CONSTANTS_LOGGING_QUEUE_SIZE_INTERVAL`, `FLOW_CONTROL_CREDIT_POLL_INTERVAL_IN_MS`, etc.), the test will fail even though `ApplicationConfig` is behaving correctly. Consider adding an `ifUnset(envVar)(...)` helper and wrapping each exact-value assertion (or update the ScalaDoc if you intentionally want the test to require a clean env).
##########
common/config/src/test/scala/org/apache/texera/common/config/EnvironmentalVariableSpec.scala:
##########
@@ -0,0 +1,133 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.texera.common.config
+
+import org.scalatest.flatspec.AnyFlatSpec
+import org.scalatest.matchers.should.Matchers
+
+/**
+ * Spec for [[EnvironmentalVariable]], the registry of environment-variable *names* used across
+ * the *.conf files. Referencing every constant forces the whole object to initialize, so a
+ * deleted or renamed constant surfaces here; asserting distinctness guards against two settings
+ * accidentally sharing one env-var name.
+ */
+class EnvironmentalVariableSpec extends AnyFlatSpec with Matchers {
+
+ import EnvironmentalVariable._
+
+ private val allNames: Seq[String] = Seq(
+ ENV_JAVA_OPTS,
+ ENV_FILE_SERVICE_GET_PRESIGNED_URL_ENDPOINT,
+ ENV_FILE_SERVICE_UPLOAD_ONE_FILE_TO_DATASET_ENDPOINT,
+ ENV_USER_JWT_TOKEN,
+ ENV_AUTH_JWT_SECRET,
+ ENV_JDBC_URL,
+ ENV_JDBC_USERNAME,
+ ENV_JDBC_PASSWORD,
+ ENV_ICEBERG_CATALOG_TYPE,
+ ENV_ICEBERG_CATALOG_REST_URI,
+ ENV_ICEBERG_CATALOG_REST_WAREHOUSE_NAME,
+ ENV_ICEBERG_CATALOG_POSTGRES_URI_WITHOUT_SCHEME,
+ ENV_ICEBERG_CATALOG_POSTGRES_USERNAME,
+ ENV_ICEBERG_CATALOG_POSTGRES_PASSWORD,
+ ENV_ICEBERG_TABLE_RESULT_NAMESPACE,
+ ENV_ICEBERG_TABLE_CONSOLE_MESSAGES_NAMESPACE,
+ ENV_ICEBERG_TABLE_RUNTIME_STATISTICS_NAMESPACE,
+ ENV_ICEBERG_TABLE_STATE_NAMESPACE,
+ ENV_ICEBERG_TABLE_COMMIT_BATCH_SIZE,
+ ENV_ICEBERG_TABLE_COMMIT_NUM_RETRIES,
+ ENV_ICEBERG_TABLE_COMMIT_MIN_WAIT_MS,
+ ENV_ICEBERG_TABLE_COMMIT_MAX_WAIT_MS,
+ ENV_LAKEFS_ENDPOINT,
+ ENV_LAKEFS_AUTH_API_SECRET,
+ ENV_LAKEFS_AUTH_USERNAME,
+ ENV_LAKEFS_AUTH_PASSWORD,
+ ENV_LAKEFS_BLOCK_STORAGE_TYPE,
+ ENV_LAKEFS_BLOCK_STORAGE_BUCKET_NAME,
+ ENV_S3_ENDPOINT,
+ ENV_S3_REGION,
+ ENV_S3_AUTH_USERNAME,
+ ENV_S3_AUTH_PASSWORD,
+ ENV_CONSTANTS_LOGGING_QUEUE_SIZE_INTERVAL,
+ ENV_CONSTANTS_NUM_WORKER_PER_OPERATOR,
+ ENV_CONSTANTS_MAX_RESOLUTION_ROWS,
+ ENV_CONSTANTS_MAX_RESOLUTION_COLUMNS,
+ ENV_CONSTANTS_STATUS_UPDATE_INTERVAL,
+ ENV_FLOW_CONTROL_MAX_CREDIT_ALLOWED_IN_BYTES_PER_CHANNEL,
+ ENV_FLOW_CONTROL_CREDIT_POLL_INTERVAL_IN_MS,
+ ENV_NETWORK_BUFFERING_DEFAULT_DATA_TRANSFER_BATCH_SIZE,
+ ENV_NETWORK_BUFFERING_ENABLE_ADAPTIVE_BUFFERING,
+ ENV_NETWORK_BUFFERING_ADAPTIVE_BUFFERING_TIMEOUT_MS,
+ ENV_RECONFIGURATION_ENABLE_TRANSACTIONAL_RECONFIGURATION,
+ ENV_CACHE_ENABLED,
+ ENV_USER_SYS_ENABLED,
+ ENV_USER_SYS_GOOGLE_CLIENT_ID,
+ ENV_USER_SYS_GOOGLE_SMTP_GMAIL,
+ ENV_USER_SYS_GOOGLE_SMTP_PASSWORD,
+ ENV_USER_SYS_VERSION_TIME_LIMIT_IN_MINUTES,
+ ENV_RESULT_CLEANUP_TTL_IN_SECONDS,
+ ENV_RESULT_CLEANUP_COLLECTION_CHECK_INTERVAL_IN_SECONDS,
+ ENV_WEB_SERVER_WORKFLOW_STATE_CLEANUP_IN_SECONDS,
+ ENV_WEB_SERVER_PYTHON_CONSOLE_BUFFER_SIZE,
+ ENV_WEB_SERVER_WORKFLOW_RESULT_PULLING_IN_SECONDS,
+ ENV_WEB_SERVER_CLEAN_ALL_EXECUTION_RESULTS_ON_SERVER_START,
+ ENV_MAX_WORKFLOW_WEBSOCKET_REQUEST_PAYLOAD_SIZE_KB,
+ ENV_FAULT_TOLERANCE_LOG_STORAGE_URI,
+ ENV_FAULT_TOLERANCE_LOG_FLUSH_INTERVAL_MS,
+ ENV_FAULT_TOLERANCE_LOG_RECORD_MAX_SIZE_IN_BYTE,
+ ENV_FAULT_TOLERANCE_MAX_SUPPORTED_RESEND_QUEUE_LENGTH,
+ ENV_FAULT_TOLERANCE_DELAY_BEFORE_RECOVERY,
+ ENV_FAULT_TOLERANCE_HDFS_STORAGE_ADDRESS,
+ ENV_SCHEDULE_GENERATOR_ENABLE_COST_BASED_SCHEDULE_GENERATOR,
+ ENV_SCHEDULE_GENERATOR_MAX_CONCURRENT_REGIONS,
+ ENV_SCHEDULE_GENERATOR_USE_GLOBAL_SEARCH,
+ ENV_SCHEDULE_GENERATOR_USE_TOP_DOWN_SEARCH,
+ ENV_SCHEDULE_GENERATOR_SEARCH_TIMEOUT_MILLISECONDS,
+ ENV_AI_ASSISTANT_SERVER_ASSISTANT,
+ ENV_AI_ASSISTANT_SERVER_AI_SERVICE_KEY,
+ ENV_AI_ASSISTANT_SERVER_AI_SERVICE_URL
+ )
+
+ "EnvironmentalVariable" should "expose non-empty, unique env-var names" in {
+ allNames.foreach(name => name.trim should not be empty)
+ allNames.distinct.size shouldBe allNames.size
+ }
+
+ it should "use the documented names for a representative sample" in {
+ ENV_JAVA_OPTS shouldBe "JAVA_OPTS"
+ ENV_AUTH_JWT_SECRET shouldBe "AUTH_JWT_SECRET"
+ ENV_JDBC_URL shouldBe "STORAGE_JDBC_URL"
+ ENV_S3_ENDPOINT shouldBe "STORAGE_S3_ENDPOINT"
+ ENV_CACHE_ENABLED shouldBe "CACHE_ENABLED"
+ }
+
+ "EnvironmentalVariable.get" should "return None for an unset variable" in {
+ val absent = "TEXERA_DEFINITELY_UNSET_" + System.nanoTime().toString
+ EnvironmentalVariable.get(absent) shouldBe None
+ }
+
+ it should "return Some(value) for a variable that is set in the JVM environment" in {
+ val env = System.getenv()
+ if (!env.isEmpty) {
+ val entry = env.entrySet().iterator().next()
+ EnvironmentalVariable.get(entry.getKey) shouldBe Some(entry.getValue)
+ }
+ }
Review Comment:
This test can become a no-op (and still pass) if `System.getenv()` is empty, which means it may not actually validate the `Some(value)` case. Prefer explicitly canceling/skipping the test when the environment is empty so the suite doesn’t report a false-positive pass without making any assertion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4880658817
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 4 worse · ⚪ 11 noise (<±5%) · 0 without baseline
> Compared against main `3dd4c2b` benchmarked on **this same runner**, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.
[Dashboard](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/28694929808)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| 🔴 | bs=10 sw=10 sl=64 | 371 | 0.226 | 25,442/47,832/47,832 us | 🔴 +6.5% / 🔴 +212.0% |
| ⚪ | bs=100 sw=10 sl=64 | 827 | 0.505 | 119,478/150,340/150,340 us | ⚪ within ±5% / 🔴 +38.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 923 | 0.563 | 1,083,463/1,130,338/1,130,338 us | ⚪ within ±5% / 🔴 +9.5% |
<details><summary>Baseline details</summary>
Latest main `3dd4c2b` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 371 tuples/sec | 395 tuples/sec | 772.08 tuples/sec | -6.1% | -51.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.226 MB/s | 0.241 MB/s | 0.471 MB/s | -6.2% | -52.0% |
| bs=10 sw=10 sl=64 | p50 | 25,442 us | 24,733 us | 12,745 us | +2.9% | +99.6% |
| bs=10 sw=10 sl=64 | p95 | 47,832 us | 44,909 us | 15,330 us | +6.5% | +212.0% |
| bs=10 sw=10 sl=64 | p99 | 47,832 us | 44,909 us | 19,054 us | +6.5% | +151.0% |
| bs=100 sw=10 sl=64 | throughput | 827 tuples/sec | 818 tuples/sec | 982.64 tuples/sec | +1.1% | -15.8% |
| bs=100 sw=10 sl=64 | MB/s | 0.505 MB/s | 0.499 MB/s | 0.6 MB/s | +1.2% | -15.8% |
| bs=100 sw=10 sl=64 | p50 | 119,478 us | 124,169 us | 101,961 us | -3.8% | +17.2% |
| bs=100 sw=10 sl=64 | p95 | 150,340 us | 144,015 us | 108,335 us | +4.4% | +38.8% |
| bs=100 sw=10 sl=64 | p99 | 150,340 us | 144,015 us | 114,379 us | +4.4% | +31.4% |
| bs=1000 sw=10 sl=64 | throughput | 923 tuples/sec | 916 tuples/sec | 1,013 tuples/sec | +0.8% | -8.9% |
| bs=1000 sw=10 sl=64 | MB/s | 0.563 MB/s | 0.559 MB/s | 0.618 MB/s | +0.7% | -9.0% |
| bs=1000 sw=10 sl=64 | p50 | 1,083,463 us | 1,090,078 us | 993,573 us | -0.6% | +9.0% |
| bs=1000 sw=10 sl=64 | p95 | 1,130,338 us | 1,148,469 us | 1,032,489 us | -1.6% | +9.5% |
| bs=1000 sw=10 sl=64 | p99 | 1,130,338 us | 1,148,469 us | 1,065,526 us | -1.6% | +6.1% |
</details>
<details><summary>Raw CSV</summary>
```csv
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,539.22,200,128000,371,0.226,25441.50,47831.55,47831.55
1,100,10,64,20,2417.10,2000,1280000,827,0.505,119477.60,150339.54,150339.54
2,1000,10,64,20,21669.33,20000,12800000,923,0.563,1083463.46,1130337.57,1130337.57
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4881078778
Thanks — done both.
`allNames` is now built reflectively (every no-arg `String` accessor named `ENV_*` on the object), so a newly added constant is picked up automatically and inherits the well-formedness and uniqueness checks for free — no hand-maintained mirror to drift.
I also swapped `name.trim should not be empty` for `fullyMatch regex "[A-Z][A-Z0-9_]*"`, which rejects the padded-name case (`"STORAGE_JDBC_URL "`) as well as all-whitespace. Pushed in 0a73119.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] test(config): add unit test coverage for ApplicationConfig, GuiConfig, and EnvironmentalVariable [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on PR #6083:
URL: https://github.com/apache/texera/pull/6083#issuecomment-4881028100
One forward-looking note on `EnvironmentalVariableSpec`: the `allNames` list is a hand-maintained mirror of the object, so a newly added `ENV_*` constant silently falls outside every assertion and the uniqueness check erodes over time. Enumerating the constants reflectively (all no-arg `String` members named `ENV_*`) would make the spec self-updating. While there, `name.trim should not be empty` only rejects all-whitespace names — a padded name like `"STORAGE_JDBC_URL "` still passes, which is the sneakier failure (env lookups silently miss); asserting `name shouldBe name.trim` or a `^[A-Z][A-Z0-9_]*$` shape check would cover both. Fine as a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org