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

[BEAM-14334] Remove remaining forkEvery 1 from all Spark tests and stop mixing unit tests with runner validations. #17662

Merged

Conversation

mosche
Copy link
Member

@mosche mosche commented May 13, 2022

This is a follow up of #17406 fixing the same issue for all remaining testing tasks. That is basically the hadoop compatibility tests and the runner validations.

Additionally this removes the current intransparent mixing of unit tests and runner validations. All tests of the project are run as part of the test task (and the hadoop compatibility tests), whereas runner validations are defined in testing only.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • Update CHANGES.md with noteworthy changes.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

…op mixing unit tests with runner validations.
@mosche
Copy link
Member Author

mosche commented May 13, 2022

R: @aromanenko-dev
R: @echauchot


classpath = configurations.validatesRunner
testClassesDirs = files(
project(":sdks:java:core").sourceSets.test.output.classesDirs,
project(":runners:core-java").sourceSets.test.output.classesDirs,
)
testClassesDirs += files(project.sourceSets.test.output.classesDirs)
Copy link
Member Author

Choose a reason for hiding this comment

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

All unit tests should be run as part of the test task

maxParallelForks 4
useJUnit {
includeCategories 'org.apache.beam.sdk.testing.ValidatesRunner'
includeCategories 'org.apache.beam.runners.spark.UsesCheckpointRecovery'
Copy link
Member Author

Choose a reason for hiding this comment

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

Unit test!

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

as said above, tests of this custom category are normal unit tests and are already run during test. there's no runner validation for such a category

@aromanenko-dev
Copy link
Contributor

Run Spark ValidatesRunner

@aromanenko-dev
Copy link
Contributor

Run Spark StructuredStreaming ValidatesRunner

Copy link
Contributor

@aromanenko-dev aromanenko-dev left a comment

Choose a reason for hiding this comment

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

Thanks! It LGTM, just several minor questions, ptal

@@ -48,6 +48,17 @@ configurations {
examplesJavaIntegrationTest
}

def sparkTestProperties(overrides = [:]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it just refactoring (extract method)?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes 👍

@@ -113,10 +117,6 @@ test {
}

maxParallelForks 4
useJUnit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it was removed?

Copy link
Member Author

@mosche mosche May 17, 2022

Choose a reason for hiding this comment

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

previously these unit tests were excluded during test runs because they failed. instead these were run as part of validatesRunnerStreaming.
I fixed them so they can be run as normal unit tests, so this exclusion isn't needed any more.

maxParallelForks 4
useJUnit {
includeCategories 'org.apache.beam.sdk.testing.ValidatesRunner'
includeCategories 'org.apache.beam.runners.spark.UsesCheckpointRecovery'
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean?

@mosche
Copy link
Member Author

mosche commented May 17, 2022

Run Spark ValidatesRunner

1 similar comment
@mosche
Copy link
Member Author

mosche commented May 17, 2022

Run Spark ValidatesRunner

@mosche mosche closed this May 17, 2022
@mosche mosche reopened this May 17, 2022
@mosche
Copy link
Member Author

mosche commented May 17, 2022

Run Java PreCommit

1 similar comment
@mosche
Copy link
Member Author

mosche commented May 17, 2022

Run Java PreCommit

@mosche
Copy link
Member Author

mosche commented May 17, 2022

precommit issues are unrelated

@aromanenko-dev
Copy link
Contributor

Agree, it's not related to this PR

@aromanenko-dev aromanenko-dev merged commit 8fb55ef into apache:master May 17, 2022
@mosche mosche deleted the BEAM-14334-RemoveRemainingForkEvery1 branch July 22, 2022 11:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants