-
Notifications
You must be signed in to change notification settings - Fork 137
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
Test Support: Get Packager from Dependencies #897
Test Support: Get Packager from Dependencies #897
Conversation
This utility function allows an app to load an Embroider packager dynamically from their dependencies. This is useful for testing, where we want to run the same tests against different packagers to verify that the packagers actually work. Tastes great with `scenario-tester`!
Once thing worth visiting as part of reviewing/working on this code; @ef4 mentioned something specific when I suggested this idea last week
This does expand the packager matrix by the "ember version" matrix, though that makes sense given the fact that the packager matrix only contains Webpack currently. Once the Vite packager is added to the mix, we'd need some way to avoid generating every possible job -- does Scenario Tester have an API for ignoring particular combinations? |
release, | ||
}) | ||
.expand({ | ||
packager_webpack, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Point maybe worth bike-shedding; using packager_webpack
rather than just webpack
means that the whole packager_webpack
string is in the name of the CI job, etc. That's nice and specific but kind of verbose, in case it matters to anyone 🤷♂️
@alexlafroscia It's possible to filter the matrix before we hand it off to github: embroider/test-packages/support/suite-setup-util.ts Lines 83 to 97 in d8edf75
|
lts_3_24, | ||
release, | ||
}) | ||
.expand({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is overkill and we should just add vite to this list:
export function supportMatrix(scenarios: Scenarios) {
return scenarios.expand({
lts_3_16,
lts_3_20,
lts_3_24,
release,
vite
});
The vite scenario would use the default ember version. If we later find some particular combinations that are problematic and need regression coverage, we can explicitly add those combinations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, cool -- I can tweak it to work that way instead!
Should each of the "Ember Version" variants explicitly add the Embroider packager, and we'll remove the Webpack "variant" altogether?
Going to close this, because while I expect to be adding additional packagers pretty soon, I don't anticipate doing it inside the broccoli pipeline. We want to invert control, so that you run them directly and they have plugins that call out to the embroider-specific things. That would change the way this test setup would work. |
As discussed in #759, we need some way to expand the tests to cover the behavior of packagers besides the existing Webpack one. We could do this by having the build-a-real-app tests load their packager dynamically based on locating a known packager in their dependencies, then use
scenario-tester
to configure scenarios with different packagers installed.This PR sets up that infrastructure and configures the
app-template
tests to use it.