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

ci: Move full compat matrix tests for local-server and tinylicious to the e2e tests pipeline #20057

Merged
merged 4 commits into from
Mar 15, 2024

Conversation

alexvy86
Copy link
Contributor

@alexvy86 alexvy86 commented Mar 9, 2024

Description

This moves the execution of full-compat-matrix e2e tests from the build-client pipeline to the e2e tests pipeline. This saves time on the "first step" of our CI workflows, the actual build (spends ~3m/9m running tests for local-server/tinylicious instead of ~12m/50m).

The extra time is now spent in the e2e tests pipeline (e.g. before ~3m/12m for local-server/tinylicious, and after, ~6m/40m).

The times above reflect specifically the time for the pipeline step that runs the tests, not the whole stage.

Note about previous version of PR

An initial version of this PR exposed a flag in the ADO UI so we could do "non-full-compat-matrix" testing for build - client in the internal project, to match what we do in the public project. That's still an option if we don't like moving the full-compat-matrix tests to the e2e tests pipeline only.

Description of that version This exposes a new checkbox when manually running the `Build - client packages` pipeline in the internal ADO project, to allow us to kick off runs that don't run local-server/tinylicious tests with the **full** compat matrix (still uses some level of compat, but I'm not familiar enough to understand which kind exactly; it's the same one used by PR builds):

image

So we can have runs that take this much time to run tests:

image

Instead of this much:

image

This is particularly useful when running Build - client packages not for the sake of generating artifacts to be released publicly, but for testing changes to the pipeline itself, or just need artifacts in order to test changes to a subsequent pipeline (e.g. one of our several test pipelines that require the published packages from Build - client packages).

Sample run (msft internal).

Reviewer Guidance

The review process is outlined on this wiki page.

@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Mar 9, 2024
@alexvy86 alexvy86 requested a review from a team March 9, 2024 03:27
- ci:test:realsvc:local
- ci:test:realsvc:tinylicious
- ${{ if ne(variables['System.TeamProject'], 'public') }}:
- ${{ else }}:
Copy link
Contributor

Choose a reason for hiding this comment

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

The only reason these were added because we shipped some serious bugs. I don't remember how bad they were, but it was a bad customer experience. It specifically tests loader and driver compat. I would recommend you move these tests to the e2e test pipelines as they aren't run there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable, I'll look into that. My take is that we can probably remove them here if we move them, since we do monitor the e2e tests pipeline closely to keep tinylicious test pass rate at 100%; we would still get notified even if a single test started failing over there.

@alexvy86 alexvy86 changed the title ci: Allow internal runs of build - client to run tests without full compat ci: Move full compat matrix tests for local-server and tinylicious to the e2e tests pipeline Mar 14, 2024
Comment on lines -47 to -52
"ci:test:realsvc:local:full": "pnpm run -r --no-sort --stream --no-bail test:realsvc:local:report:full",
"ci:test:realsvc:local:full:coverage": "c8 --no-clean pnpm recursive --no-sort --stream --no-bail run test:realsvc:local:report:full",
"ci:test:realsvc:tinylicious": "pnpm run -r --no-sort --stream --no-bail test:realsvc:tinylicious:report",
"ci:test:realsvc:tinylicious:coverage": "c8 --no-clean pnpm run -r --no-sort --stream --no-bail test:realsvc:tinylicious:report ",
"ci:test:realsvc:tinylicious:full": "pnpm run -r --no-sort --stream --no-bail test:realsvc:tinylicious:report:full",
"ci:test:realsvc:tinylicious:full:coverage": "c8 --no-clean pnpm run -r --no-sort --stream --no-bail test:realsvc:tinylicious:report:full",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These four scripts became obsolete because the build-client and test-stability pipelines now only run their corresponding non-:full versions. The :full scripts now only exist in packages/test/test-end-to-end-tests/package.json since that package itself is the only one that needs to run them (when it runs in the e2e tests pipeline).

@alexvy86 alexvy86 merged commit 2311ce4 into microsoft:main Mar 15, 2024
31 checks passed
@alexvy86 alexvy86 deleted the make-full-compat-optional branch March 15, 2024 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants