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

pants ci: add integration tests jobs #6273

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

pants ci: add integration tests jobs #6273

wants to merge 8 commits into from

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Nov 2, 2024

This adds 2 GHA jobs to the pants+pytest CI workflow:

  • integration-tests
  • integration-st2cluster-tests

Sadly, GHA is very repetitive, so a lot of the steps for these jobs are just copies of steps in other jobs. So, this should be fairly easy to review despite the line count. Go through each commit to see why I made a few changes to some steps.

Currently, we run Makefile-based integration tests in CI and Orquesta CI workflows. For the pants+pytest CI, I opted to do 2 jobs instead of separate workflows. Also, I found that the only integration tests that require a running st2cluster (started with tools/launchdev.sh) are the orquesta tests. Since st2cluster is not required by most integration tests, I was able to simplify the setup and avoid starting it. This also means there will be far fewer logs to go through when looking up failures about one of these integration tests. So, I used that requirement (needs a running st2cluster) as the dividing line between the 2 sets of integration tests. By using "st2cluster" instead of "orquesta" in the job name, I created an (hopefully) obvious home for future non-orquesta tests that need to use st2cluster.

These PRs were split out of this PR to keep each PR focused and easier to review:

@cognifloyd cognifloyd added this to the pants milestone Nov 2, 2024
@cognifloyd cognifloyd self-assigned this Nov 2, 2024
@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Nov 2, 2024
@cognifloyd cognifloyd force-pushed the pants-itests branch 10 times, most recently from 6575608 to 7f869b9 Compare November 3, 2024 06:23
@pull-request-size pull-request-size bot added size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. and removed size/L PR that changes 100-499 lines. Requires some effort to review. labels Nov 6, 2024
@cognifloyd cognifloyd force-pushed the pants-itests branch 3 times, most recently from 1d377cb to 11b7e65 Compare November 7, 2024 03:04
@pull-request-size pull-request-size bot added size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. and removed size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review. labels Nov 8, 2024
@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. labels Nov 9, 2024
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Nov 9, 2024
@cognifloyd
Copy link
Member Author

cognifloyd commented Nov 15, 2024

Planned sequence of PRs:

flowchart LR
    A("✅ #6275: BUILD metadata") --> D("✅ #6279: pants-plugins/uses_services st2cluster")
    D ---> H("⏳ #6273: Add integration tests job")
    B("✅ #6276: Refactor CI scripts") ---> G("✅ #6283: Add redis namespace + ST2_* ST2TESTS_* vars in launchdev.sh")
    G --> H
    C("✅ #6277: env var support") --> E("✅ #6278: pants-plugins/uses_services refactor + mongo")
    E --> F("✅ #6282: Add messaging.prefix option") & G
    F --> H
Loading

Copy link
Contributor

@winem winem left a comment

Choose a reason for hiding this comment

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

Well done!

This is btw the first time that I used the Explain feature of the GItHub Copilot which was quite helpful to double-check my own interpretation of the code.

pants `--tag=...` notes:
`--tags=abc,def` (a csv list):
    is an OR condition (either `abc` tag, or `def` tag, or both).
`--tags=abc --tag=def` (multiple entries):
    is an AND condition (Both `abc` tag AND `def` tag).

So, we use `--tag=integration --tag=-st2cluster` to select targets
that have the `integration` tag AND do NOT have the `st2cluster` tag.
pants `--tag=...` notes:
`--tags=abc,def` (a csv list):
    is an OR condition (either `abc` tag, or `def` tag, or both).
`--tags=abc --tag=def` (multiple entries):
    is an AND condition (Both `abc` tag AND `def` tag).

So, we use `--tag=integration --tag=st2cluster` to select targets
that have the `integration tag AND the `st2cluster` tag.
(The other integration test job excludes the st2cluster tagged targets).
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/XXL PR that changes 1000+ lines. You should absolutely split your PR into several. labels Nov 29, 2024
@cognifloyd cognifloyd changed the title pants ci: add integration tests job pants ci: add integration tests jobs Nov 29, 2024
@cognifloyd cognifloyd marked this pull request as ready for review November 29, 2024 06:30
Comment on lines +261 to +262
"ST2_CI",
"ST2_CI_RUN_ORQUESTA_PAUSE_RESUME_TESTS",
Copy link
Member Author

Choose a reason for hiding this comment

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

These vars are used to skip some integration tests unless running in CI.


- name: Integration Tests
env:
ST2_CI_RUN_ORQUESTA_PAUSE_RESUME_TESTS: 'true'
Copy link
Member Author

Choose a reason for hiding this comment

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

With all of the recent changes to use redis and to make running tests in parallel safer, and with pants+pytest running the tests, I attempted to run tests that we've been skipping even in CI for a while. Happily, they seem much more reliable now, so this line re-enables the tests in pants+pytest CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure: ci/cd pantsbuild size/L PR that changes 100-499 lines. Requires some effort to review. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants