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

FEDX-1987: Only generate backwards-compatible output when necessary #52

Merged
merged 8 commits into from
Nov 21, 2024

Conversation

evanweible-wf
Copy link
Contributor

@evanweible-wf evanweible-wf commented Nov 7, 2024

FEDX-1987

Issue Status

In v3, we moved the output of the browser-aggregation test config from test/dart_test.browser_aggregate.yaml to dart_test.browser_aggregate.yaml (in the root). For backwards-compatibility, we continued to output to the deprecated location, too. Most consumers should already be including the dart_test.browser_aggregate.yaml in the root instead of the copy from the test/ directory, but if they aren't, it should be trivial to detect by reading dart_test.yaml where it would need to be included.

This PR makes that change: when test/dart_test.browser_aggregate.yaml is found in dart_test.yaml, the builder will output that file. Otherwise, it will not, and only the root dart_test.browser_aggregate.yaml will be generated.

todbachman-wf
todbachman-wf previously approved these changes Nov 7, 2024
@evanweible-wf
Copy link
Contributor Author

The example tests are timing out on stable and beta because the dart run build_runner test process is not exiting even though the child test process has exited.. not sure why. I opened an issue here: dart-lang/build#3770

@evanweible-wf evanweible-wf force-pushed the smart-output-of-backwards-compat-file branch from 6ccd870 to 3e4e1be Compare November 13, 2024 17:59
@evanweible-wf
Copy link
Contributor Author

This is passing CI now with the workarounds in place.

@rmconsole6-wk rmconsole6-wk changed the title Only generate backwards-compatible output when necessary FEDX-1987 Only generate backwards-compatible output when necessary Nov 19, 2024
@bender-wk bender-wk changed the title FEDX-1987 Only generate backwards-compatible output when necessary FEDX-1987: Only generate backwards-compatible output when necessary Nov 19, 2024
todbachman-wf
todbachman-wf previously approved these changes Nov 19, 2024
final dartTestYaml = await buildStep.readAsString(dartTestYamlId);
if (dartTestYaml.contains('test/dart_test.browser_aggregate.yaml')) {
log.fine(
'Found `test/dart_test.browser_aggregate.yaml` in `dart_test.yaml`, will generate it for backwards-compatibility.');
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: What would you think about making this an info or warning log that includes a suggestion to update the dart_test.yaml file to reference the new file?

@todbachman-wf
Copy link
Member

QA +1. I pulled this into wdesk_sdk and verified that the backwards compatible file is no longer created when running the tests in browser aggregation mode.

Copy link
Member

@todbachman-wf todbachman-wf left a comment

Choose a reason for hiding this comment

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

+10

@robbecker-wf
Copy link
Member

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

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

+1 from RM

@rmconsole3-wf rmconsole3-wf merged commit 4413c76 into master Nov 21, 2024
4 checks passed
@rmconsole3-wf rmconsole3-wf deleted the smart-output-of-backwards-compat-file branch November 21, 2024 21:15
@patkujawa-wf
Copy link

Something recently started causing CI failures like https://github.com/Workiva/tasker-ui/actions/runs/11962147002 due to

[INFO] build_web_compilers:entrypoint on test/templates/test_template.browser_aggregate_test.dart.browser_test.dart:Info: Compiling without sound null safety!
Dart 3 will only support sound null safety, see https://dart.dev/null-safety
org-dartlang-app:///packages/test_api/src/frontend/fake.dart:49:11:
Hint: This 'noSuchMethod' implementation is guaranteed to throw an exception. The generated code will be smaller if it is rewritten.
Error on line 8, column 10 of dart_test.yaml: Cannot open file, path = './test/dart_test.browser_aggregate.yaml' (OS Error: No such file or directory, errno = 2)

8 │ include: test/dart_test.browser_aggregate.yaml
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Rewrite to 'noSuchMethod(Invocation i) => super.noSuchMethod(i);'.
dynamic noSuchMethod(Invocation invocation) {
^
org-dartlang-app:///packages/mocktail/src/mocktail.dart:111:11:
Hint: Overriding 'noSuchMethod' causes the compiler to generate more code and prevents the compiler from doing some optimizations.
Consider removing this 'noSuchMethod' implementation.
dynamic noSuchMethod(Invocation invocation) {
^
Compiled 71,268,909 characters Dart to 8,423,141 characters JavaScript in 169 seconds using 3068.316 MB of memory
Dart file org-dartlang-app:///test/templates/test_template.browser_aggregate_test.dart.browser_test.dart compiled to JavaScript: test/templates/test_template.browser_aggregate_test.dart.browser_test.dart.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants