Repository navigation
Conversation
The integration tests passed a copy of the swappable components API through the test plugin, but `core.swappable-components` is owned by the app plugin, so the app rejected it with API_FACTORY_CONFLICT and used its own, identical implementation. Drop the copy; the tests still exercise the overrides through the app plugin's API. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Marat Dyatko <maratd@spotify.com>
…erTestApp Both helpers ignored the errors collected while creating the test app, so a test could pass for an app that would only show its error page, for example after an API_FACTORY_CONFLICT. Throw for those errors, and keep ignoring the codes that a real app only logs as warnings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Marat Dyatko <maratd@spotify.com>
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
backstage-goalie
Bot
added
size:medium
Move DEFAULT_WARNING_CODES out of maybeCreateErrorPage into its own internal file, and import it from the test utils instead of keeping a copy, so tests fail for exactly the errors that show the app error page. Expand the changeset migration note with the API override case. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Marat Dyatko <maratd@spotify.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Marat Dyatko <maratd@spotify.com>
Rugvip
left a comment
Rugvip
left a comment
There was a problem hiding this comment.
Nice, good catch! 👍
Some thoughts around how we handle the warning codes and expected errors
|
|
||
| // Uses the same list as `createApp`, so a test fails exactly when the app | ||
| // would show its error page. | ||
| const WARNING_CODES = new Set(DEFAULT_WARNING_CODES); |
There was a problem hiding this comment.
I'm kinda thinking we should throw on these too, although it might be tricky for some of them when testing with external things that produce warnings. Perhaps we can treat them all as errors but add an "expected errors" option or such? Thinking something like this:
renderTestApp({
extensions: [...],
expectErrors: [{
code: 'EXTENSION_INPUT_DATA_IGNORED',
context: {
extensionId: 'page:foo/bar',
inputName: 'element',
}
}]
})Maybe the full context is a bit overkill and just the code is enough. We'd need to translate node and plugin in the error context to extensionId and pluginId or so as well, which make it a bit trickier. Then again you'd get that data from the failure anyway, we'd have some message like "to mark this error as expected, provide the following to the expectErrors option: ...".
All that said, is it the case that treating some of these as errors would lead to excessive errors and breakages? Maybe there's a more narrow default set of codes that are ignored, that can be tweaked with an option as well, although afaik all of these errors are things that you'd want to fix.
Hey, I just made a Pull Request!
renderInTestAppandrenderTestAppignore the errors collected while creating the test app. A test can pass for an app thatcreateAppwould replace with its error page, for example after anAPI_FACTORY_CONFLICT. This came up in review of #36332, which otherwise has to document the gap for agents and plugin authors.This PR makes both helpers throw for those errors, using the same split as
maybeCreateErrorPagein@backstage/frontend-defaults: errors fail the test, and the codes a real app only logs as warnings (such asEXTENSION_IGNORED) are still ignored.createExtensionTesteralready throws for collected errors, so this brings the other two helpers in line.To keep tests and apps from drifting apart, both use one list.
DEFAULT_WARNING_CODESmoves out ofmaybeCreateErrorPage.tsxinto its own file in@backstage/frontend-defaults(not exported, so the public API is unchanged), and@backstage/frontend-test-utilsimports it with a relative monorepo import, the same way it already imports from@backstage/frontend-app-api/src. The file is separate so the test utils don't pull in the React error page.Effect on existing tests: I ran all 49 test suites in the repo that use these helpers, plus
@backstage/frontend-defaults. Only the swappable components integration tests inplugins/appfailed. They passed a copy of thecore.swappable-componentsAPI through the test plugin, which the app has always rejected as a conflict, falling back to its own identical implementation. The first commit removes that copy; the tests still cover the overrides through the app plugin's API.Marked as BREAKING (
minorfor this0.xpackage) because tests in other repositories that currently pass with startup errors will start failing.AI disclosure: written with Claude Code.
✔️ Checklist
Signed-off-byline in the message. (more info)🤖 Generated with Claude Code