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

Run costly tests after only after merging #17956

Merged
merged 3 commits into from
Dec 4, 2023

Conversation

paulbrauner-da
Copy link
Contributor

@paulbrauner-da paulbrauner-da commented Nov 30, 2023

Context: #17812

Makes the necessary changes to build.sh, build.ps1 and the CI to run different tests on main after a merge vs against a PR. Changes two tests as a way to demonstrate the functionality.

Will tag more tests in future PRs.

@paulbrauner-da paulbrauner-da force-pushed the pb/run-costly-tests-async branch from d26a272 to 471330d Compare November 30, 2023 15:03
@paulbrauner-da paulbrauner-da changed the title Run costly tests after merging only Run costly tests after only after merging Nov 30, 2023
@paulbrauner-da paulbrauner-da force-pushed the pb/run-costly-tests-async branch from e8035bc to e8aaef6 Compare November 30, 2023 16:00
Copy link
Contributor

@garyverhaegen-da garyverhaegen-da left a comment

Choose a reason for hiding this comment

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

LGTM, but for one minor question: do we really expect to have pr-only tests? What's the use-case for that?

@paulbrauner-da
Copy link
Contributor Author

paulbrauner-da commented Dec 4, 2023

This PR showcases one use case (simplified here):

[
    da_haskell_test(
        name = "data-dependencies-{}".format(suffix),
        srcs = ["src/DA/Test/DataDependencies.hs"],
        args = test_args,
        tags = extra_tags,
    )
    for (suffix, extra_tags, test_args) in [
        # Test all LF versions on the main branch, after merging
        ("all", ["main-only"], []),
        # Don't test LF v2 when running the CI on a PR
        ("no-v2", ["pr-only"], ["--pattern !(v2)"]),
    ]
]

That is: I want to only run the non-v2 tests against PRs, and all tests against main. Ideally I'd say "if running against PR, then pass the --pattern !(v2)" flag to the test. But there is no way to express "if running against PR". Instead we can only say "define two tests: one with flag main-only and no pattern, and one with flag pr-only and some pattern". Without "pr-only" I would run the v2 tests twice on main.

I'm not sure how else to express this.

Copy link
Collaborator

@remyhaemmerle-da remyhaemmerle-da left a comment

Choose a reason for hiding this comment

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

Thanks.

@paulbrauner-da paulbrauner-da merged commit e22f8be into main Dec 4, 2023
18 checks passed
@paulbrauner-da paulbrauner-da deleted the pb/run-costly-tests-async branch December 4, 2023 09:52
@garyverhaegen-da
Copy link
Contributor

Without "pr-only" I would run the v2 tests twice on main.

Wouldn't caching make the second run instant? Or would Bazel consider them separate tests and thus not reuse the cache?

@paulbrauner-da
Copy link
Contributor Author

paulbrauner-da commented Dec 6, 2023

If I'm not mistaken they would be considered separate runs: it's the same binary, but with different flags: in one case the flag is "run all test cases", in the other case it's "run a subset of the test cases". But bazel is not aware of the semantics of the flags, so it doesn't know that one is the subset of the other.

What I could do is have two tests that run exclusive subsets instead I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants