Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests #27754

Closed
wants to merge 3 commits into from
Closed

[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests #27754

wants to merge 3 commits into from

Conversation

JoshRosen
Copy link
Contributor

What changes were proposed in this pull request?

This patch fixes several incorrect uses of assume() in our tests.

If a call to assume(condition) fails then it will cause the test to be marked as skipped instead of failed: this feature allows test cases to be skipped if certain prerequisites are missing. For example, we use this to skip certain tests when running on Windows (or when Python dependencies are unavailable).

In contrast, assert(condition) will fail the test if the condition doesn't hold.

If assume() is accidentally substituted for assert()then the resulting test will be marked as skipped in cases where it should have failed, undermining the purpose of the test.

This patch fixes several such cases, replacing certain assume() calls with assert().

Credit to @ahirreddy for spotting this problem.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

@JoshRosen JoshRosen added the TESTS label Mar 2, 2020
HyukjinKwon
HyukjinKwon reviewed Mar 2, 2020
View reviewed changes
core/src/test/scala/org/apache/spark/storage/LocalDirsSuite.scala Outdated
@@ -39,7 +39,7 @@ class LocalDirsSuite extends SparkFunSuite with BeforeAndAfter {

private def assumeNonExistentAndNotCreatable(f: File): Unit = {
try {
assume(!f.exists() && !f.mkdirs())
assert(!f.exists() && !f.mkdirs())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshRosen, this assume was intentional (see https://github.com/apache/spark/pull/17987/files#r116532209) because /NENEXISTENT_PATH can be a valid path. We should ideally fix the tests to make it invalid paths in all OSs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay reverting this line for now too as it will be tricky to find out one invalid path in "both Windows and UNIX-like systems (e.g. Linux, Mac OS)".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted this line.

@SparkQA
Copy link

SparkQA commented Mar 2, 2020

Test build #119147 has finished for PR 27754 at commit 91e0ae2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member

maropu commented Mar 2, 2020

note: I've double-checked them by a command find . -type f | grep -e "\Suite.scala$" | xargs grep "assume("
https://gist.github.com/maropu/f1c0fdd2a22609f87ae0aae9c75df891

@SparkQA
Copy link

SparkQA commented Mar 2, 2020

Test build #119157 has finished for PR 27754 at commit 082f590.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

dongjoon-hyun
dongjoon-hyun reviewed Mar 2, 2020
View reviewed changes
...main/scala/org/apache/spark/scheduler/cluster/mesos/MesosCoarseGrainedSchedulerBackend.scala Outdated
@@ -637,7 +637,7 @@ private[spark] class MesosCoarseGrainedSchedulerBackend(
if (state.equals(TaskState.RUNNING) &&
shuffleServiceEnabled &&
!slave.shuffleRegistered) {
assume(mesosExternalShuffleClient.isDefined,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshRosen . Since this is not a test case, could you remove [TESTS] in the PR title?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've rolled back this non-test change, so PR is now test-only.

dongjoon-hyun
dongjoon-hyun approved these changes Mar 2, 2020
View reviewed changes
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.
Thank you for reverting the Mesos change.
I'll merge this since all the other tests already passed.
Merged to master/3.0/2.4. Thank you all!

dongjoon-hyun pushed a commit that referenced this pull request Mar 2, 2020
@JoshRosen @dongjoon-hyun
[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests
8cb23f0 
### What changes were proposed in this pull request?

This patch fixes several incorrect uses of `assume()` in our tests.

If a call to `assume(condition)` fails then it will cause the test to be marked as skipped instead of failed: this feature allows test cases to be skipped if certain prerequisites are missing. For example, we use this to skip certain tests when running on Windows (or when Python dependencies are unavailable).

In contrast, `assert(condition)` will fail the test if the condition doesn't hold.

If `assume()` is accidentally substituted for `assert()`then the resulting test will be marked as skipped in cases where it should have failed, undermining the purpose of the test.

This patch fixes several such cases, replacing certain `assume()` calls with `assert()`.

Credit to ahirreddy for spotting this problem.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

Existing tests.

Closes #27754 from JoshRosen/fix-assume-vs-assert.

Lead-authored-by: Josh Rosen <rosenville@gmail.com>
Co-authored-by: Josh Rosen <joshrosen@databricks.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit f0010c8)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
dongjoon-hyun pushed a commit that referenced this pull request Mar 2, 2020
@JoshRosen @dongjoon-hyun
[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests
0b71b4d 
### What changes were proposed in this pull request?

This patch fixes several incorrect uses of `assume()` in our tests.

If a call to `assume(condition)` fails then it will cause the test to be marked as skipped instead of failed: this feature allows test cases to be skipped if certain prerequisites are missing. For example, we use this to skip certain tests when running on Windows (or when Python dependencies are unavailable).

In contrast, `assert(condition)` will fail the test if the condition doesn't hold.

If `assume()` is accidentally substituted for `assert()`then the resulting test will be marked as skipped in cases where it should have failed, undermining the purpose of the test.

This patch fixes several such cases, replacing certain `assume()` calls with `assert()`.

Credit to ahirreddy for spotting this problem.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

Existing tests.

Closes #27754 from JoshRosen/fix-assume-vs-assert.

Lead-authored-by: Josh Rosen <rosenville@gmail.com>
Co-authored-by: Josh Rosen <joshrosen@databricks.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
(cherry picked from commit f0010c8)
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@SparkQA
Copy link

SparkQA commented Mar 3, 2020

Test build #119190 has finished for PR 27754 at commit 84f5105.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@JoshRosen JoshRosen deleted the fix-assume-vs-assert branch March 4, 2020 18:51
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
@JoshRosen @sjincho
[SPARK-31003][TESTS] Fix incorrect uses of assume() in tests
e72d7b5 
### What changes were proposed in this pull request?

This patch fixes several incorrect uses of `assume()` in our tests.

If a call to `assume(condition)` fails then it will cause the test to be marked as skipped instead of failed: this feature allows test cases to be skipped if certain prerequisites are missing. For example, we use this to skip certain tests when running on Windows (or when Python dependencies are unavailable).

In contrast, `assert(condition)` will fail the test if the condition doesn't hold.

If `assume()` is accidentally substituted for `assert()`then the resulting test will be marked as skipped in cases where it should have failed, undermining the purpose of the test.

This patch fixes several such cases, replacing certain `assume()` calls with `assert()`.

Credit to ahirreddy for spotting this problem.

### Does this PR introduce any user-facing change?

No.

### How was this patch tested?

Existing tests.

Closes apache#27754 from JoshRosen/fix-assume-vs-assert.

Lead-authored-by: Josh Rosen <rosenville@gmail.com>
Co-authored-by: Josh Rosen <joshrosen@databricks.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maropu maropu maropu approved these changes

@srowen srowen srowen approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

@dongjoon-hyun dongjoon-hyun dongjoon-hyun approved these changes

@Ngone51 Ngone51 Ngone51 approved these changes

Assignees
No one assigned
Labels
TESTS
Projects
None yet
Milestone
No milestone
7 participants
@JoshRosen @SparkQA @maropu @srowen @HyukjinKwon @dongjoon-hyun @Ngone51