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

Prevent certain tests from being run twice in CI #4529

Merged
merged 13 commits into from
Jul 24, 2023

Conversation

stxue1
Copy link
Contributor

@stxue1 stxue1 commented Jul 11, 2023

This fixes #4521. Only the batch_systems, jobstore_and_provisioning, and wdl tests are affected, as I think the integration tests and py310_main benefit from running all their current tests.

Changelog Entry

To be copied to the draft changelog by merger:

  • Prevent certain tests from being run twice in CI

Reviewer Checklist

  • Make sure it is coming from issues/XXXX-fix-the-thing in the Toil repo, or from an external repo.
    • If it is coming from an external repo, make sure to pull it in for CI with:
      contrib/admin/test-pr otheruser theirbranchname issues/XXXX-fix-the-thing
      
    • If there is no associated issue, create one.
  • Read through the code changes. Make sure that it doesn't have:
    • Addition of trailing whitespace.
    • New variable or member names in camelCase that want to be in snake_case.
    • New functions without type hints.
    • New functions or classes without informative docstrings.
    • Changes to semantics not reflected in the relevant docstrings.
    • New or changed command line options for Toil workflows that are not reflected in docs/running/{cliOptions,cwl,wdl}.rst
    • New features without tests.
  • Comment on the lines of code where problems exist with a review comment. You can shift-click the line numbers in the diff to select multiple lines.
  • Finish the review with an overall description of your opinion.

Merger Checklist

  • Make sure the PR passes tests.
  • Make sure the PR has been reviewed since its last modification. If not, review it.
  • Merge with the Github "Squash and merge" feature.
    • If there are multiple authors' commits, add Co-authored-by to give credit to all contributing authors.
  • Copy its recommended changelog entry to the Draft Changelog.
  • Append the issue number in parentheses to the changelog entry.

@stxue1 stxue1 changed the title Issues/4521 duplicate ci tests Prevent certain tests from being run twice in CI Jul 11, 2023
@adamnovak
Copy link
Member

@stxue1 it looks like this is on top of 802dd1c when it should have been on top of d686dac which is the squashed-and-merged version of the commit. You probably didn't to a git fetch origin && git checkout origin/master (assuming origin is your remote for this repo) before starting your work.

We don't want to merge this like it is, because then we'll be squashing in a bunch of duplicate copies of the commits from your last PR.

I think what you want to do is:

  1. git fetch origin
  2. git rebase -i origin/master
  3. Delete the lines for all the commits that were in your last PR (so revert accidental rename and everything before that) and save the file, and close the editor, so rebase can re-make all your commits that are for this feature on top of the current master branch.
  4. Force push the result to this PR's branch.

Copy link
Member

@adamnovak adamnovak left a comment

Choose a reason for hiding this comment

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

I think it needs rebasing, but the code looks right.

.gitlab-ci.yml Outdated Show resolved Hide resolved
Comment on lines -273 to -281
- make test tests="src/toil/test/wdl/toilwdlTest.py src/toil/test/wdl/builtinTest.py" # needs java (default-jre) to run "GATK.jar"
# Run some WDL conformance tests.
# These are not set up as Toil PyTest tests (yet?)
- git clone https://github.com/adamnovak/wdl-conformance-tests.git
- cd wdl-conformance-tests
- git checkout 0ca00774331630d8eb06990caa8a1ba178cabecb
- python run.py --runner toil-wdl-runner --versions 1.0 --threads 8
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't cutting this be part of #4530 and not this PR?

I guess it's fine to do it here though.

@stxue1 stxue1 force-pushed the issues/4521-duplicate-ci-tests branch from e0dac22 to 5b1c0c2 Compare July 17, 2023 21:27
@adamnovak adamnovak merged commit ff36711 into master Jul 24, 2023
2 checks passed
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.

CI runs tests twice
2 participants