You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "uros-b (via GitHub)" <gi...@apache.org> on 2026/08/11 11:56:08 UTC
[PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
uros-b opened a new pull request, #57927:
URL: https://github.com/apache/spark/pull/57927
### What changes were proposed in this pull request?
Rewrites 19 `assert(expr === false)` comparisons in `SecurityManagerSuite` to `assert(!expr)`.
### Why are the changes needed?
All three receivers return a Scala `Boolean` (`checkUIViewPermissions`, `checkModifyPermissions`, `aclsEnabled`), so comparing against `false` adds nothing.
The negated form also produces a better failure message. ScalaTest's `assert` macro reconstructs the source expression, so a failure reports the offending call and its argument:
```
assert(!checkUIViewPermissions("user3")) -> checkUIViewPermissions("user3") was true
assert(checkUIViewPermissions("user3") === false) -> true did not equal false
```
In a test body with a dozen near-identical permission assertions, the second message does not identify which one failed.
### Does this PR introduce _any_ user-facing change?
No. Test-only, and the assertions have identical truth values.
### How was this patch tested?
Existing suite. The rewrite is mechanical and every receiver was confirmed to be a Scala `Boolean` method, so there is no unboxing or null hazard.
The scope is the first six tests, which is where the pattern is densest; the occurrences in the later tests are left for a follow-up to keep this diff reviewable. Happy to fold in the rest if preferred.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57927:
URL: https://github.com/apache/spark/pull/57927#issuecomment-5302532499
Thank you @LuciferYang for the review and the full grep survey. To clarify a bit on the intent, it's not only about readability here. For a bool ret call, ScalaTest's assert macro only reconstructs the expression for the negated form. For example:
- `assert(securityManager.checkUIViewPermissions("user3") === false)` gives only "true did not equal false"
- `assert(!securityManager.checkUIViewPermissions("user3"))` gives better "securityManager.checkUIViewPermissions(\"user3\") was true"
So in a typical suite with many similar permission checks, the negated form names the offending call in the message itself rather than reporting an anonymous and vague `true did not equal false`.
That said, both your points make sense to me. I think that a repo wide sweep does carry risk, and wouldn't go for it. However, I would still suggest to move forward with this scoped fix at this time and improve at least this part of the code, e.g. complete conversion of `SecurityManagerSuite` and leave the rest of the tree alone.
In case you still feel strongly against it, I would be happy to close this PR as well. Please let me know!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #57927: [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite
URL: https://github.com/apache/spark/pull/57927
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #57927:
URL: https://github.com/apache/spark/pull/57927#issuecomment-5275306967
```
grep "assert" -R * | grep "== false" | awk -F ':' '{print $1}'| sort | uniq -c | sort -k1 -n -r
58 sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectDatabaseMetaDataSuite.scala
45 core/src/test/scala/org/apache/spark/SecurityManagerSuite.scala
22 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOffSuite.scala
19 streaming/src/test/scala/org/apache/spark/streaming/receiver/BlockGeneratorSuite.scala
17 sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectJdbcDataTypeSuite.scala
15 sql/core/src/test/scala/org/apache/spark/sql/execution/command/PlanResolutionSuite.scala
13 sql/core/src/test/scala/org/apache/spark/sql/DatasetSuite.scala
11 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/V2SessionCatalogSuite.scala
10 sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala
9 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CollectionExpressionsSuite.scala
9 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodegenExpressionCachingSuite.scala
9 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecisionSuite.scala
7 sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala
7 sql/core/src/test/scala/org/apache/spark/sql/execution/command/ShowTablesSuiteBase.scala
7 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastSuiteBase.scala
6 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodegenSubexpressionEliminationSuite.scala
6 core/src/test/scala/org/apache/spark/SSLOptionsSuite.scala
5 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/UpdatingSessionsIteratorSuite.scala
5 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/StateStoreCoordinatorSuite.scala
5 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DynamicPruningSubquerySuite.scala
5 mllib/src/test/scala/org/apache/spark/ml/feature/OneHotEncoderSuite.scala
5 core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
5 core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala
4 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/SymmetricHashJoinStateManagerSuite.scala
4 sql/core/src/test/scala/org/apache/spark/sql/execution/joins/HashedRelationSuite.scala
4 sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala
4 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/AttributeSetSuite.scala
4 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoderSuite.scala
4 resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStepSuite.scala
4 core/src/test/scala/org/apache/spark/util/UninterruptibleThreadSuite.scala
4 core/src/test/scala/org/apache/spark/resource/ResourceInformationSuite.scala
4 core/src/test/scala/org/apache/spark/CheckpointSuite.scala
4 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCTest.scala
4 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/V2JDBCNamespaceTest.scala
3 streaming/src/test/scala/org/apache/spark/streaming/StateMapSuite.scala
3 sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala
3 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/MapStateSuite.scala
3 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/orc/OrcSourceSuite.scala
3 sql/catalyst/src/test/scala/org/apache/spark/sql/internal/connector/PartitionPredicateImplSuite.scala
3 sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/ColumnSuite.scala
3 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/MetadataSuite.scala
3 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
3 core/src/test/scala/org/apache/spark/rdd/RDDBarrierSuite.scala
3 core/src/test/scala/org/apache/spark/MapOutputTrackerSuite.scala
2 streaming/src/test/scala/org/apache/spark/streaming/StreamingContextSuite.scala
2 streaming/src/test/scala/org/apache/spark/streaming/rdd/MapWithStateRDDSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQueryListenerSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/streaming/continuous/ContinuousQueryStatusAndProgressSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCV2Suite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/sources/ForeachWriterSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/execution/metric/SQLLastAttemptMetricUnitSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/xml/parsers/StaxXmlParserUtilsSuite.scala
2 sql/core/src/test/scala/org/apache/spark/sql/connector/ChangelogEndToEndSuite.scala
2 sql/connect/server/src/test/scala/org/apache/spark/sql/connect/service/FetchErrorDetailsHandlerSuite.scala
2 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparisonSuite.scala
2 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/StringExpressionsSuite.scala
2 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ConditionalExpressionSuite.scala
2 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeCompilerSuite.scala
2 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/encoders/RowEncoderSuite.scala
2 resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
2 mllib/src/test/scala/org/apache/spark/ml/tree/impl/RandomForestSuite.scala
2 core/src/test/scala/org/apache/spark/util/collection/AppendOnlyMapSuite.scala
2 core/src/test/scala/org/apache/spark/storage/BlockIdSuite.scala
2 core/src/test/scala/org/apache/spark/scheduler/EventLoggingListenerSuite.scala
2 core/src/test/scala/org/apache/spark/scheduler/DAGSchedulerSuite.scala
2 core/src/test/scala/org/apache/spark/resource/ResourceProfileSuite.scala
2 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/MySQLNamespaceSuite.scala
1 streaming/src/test/scala/org/apache/spark/streaming/util/WriteAheadLogSuite.scala
1 streaming/src/main/scala/org/apache/spark/streaming/scheduler/ReceiverTracker.scala
1 sql/pipelines/src/test/scala/org/apache/spark/sql/pipelines/util/SchemaInferenceUtilsSuite.scala
1 sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/ShowTablesSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/streaming/test/StreamingSinkEvolutionSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/streaming/StreamingQuerySuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/streaming/GroupStateSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/streaming/FlatMapGroupsWithStateSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfEntrySuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBStateStoreSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/PlannerSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTableCatalogSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetEncodingSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/binaryfile/BinaryFileFormatSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/ShowTablesSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLParserSuite.scala
1 sql/core/src/test/scala/org/apache/spark/sql/DataFrameStatSuite.scala
1 sql/connect/server/src/test/scala/org/apache/spark/sql/connect/service/ArtifactStatusesHandlerSuite.scala
1 sql/connect/server/src/main/scala/org/apache/spark/sql/connect/execution/ExecuteGrpcResponseSender.scala
1 sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/ClientE2ETestSuite.scala
1 sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientSuite.scala
1 sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/SparkConnectClientRetriesSuite.scala
1 sql/connect/client/jvm/src/test/scala/org/apache/spark/sql/connect/client/ArtifactSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/types/MetadataSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/plans/LogicalPlanSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/JoinSelectionHelperSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/xml/UDFXPathUtilSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/UnsafeRowConverterSuite.scala
1 sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala
1 sql/catalyst/src/main/java/org/apache/spark/sql/connector/util/V2ExpressionSQLBuilder.java
1 resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/DriverServiceFeatureStepSuite.scala
1 resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicExecutorFeatureStepSuite.scala
1 resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStepSuite.scala
1 mllib/src/test/scala/org/apache/spark/ml/regression/AFTSurvivalRegressionSuite.scala
1 mllib/src/test/scala/org/apache/spark/ml/fpm/FPGrowthSuite.scala
1 mllib/src/test/scala/org/apache/spark/ml/feature/VectorIndexerSuite.scala
1 core/src/test/scala/org/apache/spark/util/random/XORShiftRandomSuite.scala
1 core/src/test/scala/org/apache/spark/util/JsonProtocolSuite.scala
1 core/src/test/scala/org/apache/spark/SparkConfSuite.scala
1 core/src/test/scala/org/apache/spark/scheduler/SparkListenerSuite.scala
1 core/src/test/scala/org/apache/spark/scheduler/CoarseGrainedSchedulerBackendSuite.scala
1 core/src/test/scala/org/apache/spark/rdd/RDDSuite.scala
1 core/src/test/scala/org/apache/spark/executor/ExecutorSuite.scala
1 core/src/test/scala/org/apache/spark/deploy/history/FsHistoryProviderSuite.scala
1 core/src/test/scala/org/apache/spark/deploy/client/AppClientSuite.scala
1 connector/kinesis-asl/src/test/scala/org/apache/spark/streaming/kinesis/KinesisStreamSuite.scala
1 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/OracleIntegrationSuite.scala
1 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MySQLIntegrationSuite.scala
1 connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/MsSqlServerIntegrationSuite.scala
1 connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroRowReaderSuite.scala
```
1. This PR does not cover all relevant changes. There should be numerous similar instances remaining in the codebase, such as the file list shown above.
2. I recall making similar changes before, yet this tweak barely improves code readability and carries potential risks of backport conflicts.
So I do not recommend touching them intentionally. also cc @HyukjinKwon
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57927:
URL: https://github.com/apache/spark/pull/57927#issuecomment-5314784113
Thank you @LuciferYang!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58709][CORE][TESTS] Use assert(!x) instead of assert(x === false) in SecurityManagerSuite [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #57927:
URL: https://github.com/apache/spark/pull/57927#issuecomment-5314792286
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/a21b1453a39fd4b88e558d94e1a89641b8870f3c
- merged into branch-4.x https://github.com/apache/spark/commit/81b20d9690980dce71e23590ee37b0ae493aaacc
- merged into branch-4.3 https://github.com/apache/spark/commit/223b56cb91ee768afa6887a8ca5c9ed7de22d059
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org