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

♻️ Organize polyfills in prep for type-checking and core extraction #34020

Merged
merged 3 commits into from
Apr 26, 2021

Conversation

rcebulko
Copy link
Contributor

No description provided.

@rcebulko rcebulko merged commit 4f69d04 into ampproject:main Apr 26, 2021
@rcebulko rcebulko deleted the core-polyfills branch April 26, 2021 23:59
@rsimha
Copy link
Contributor

rsimha commented Apr 27, 2021

@jridgewell @rcebulko Looks like this PR introduced a silent failure in the unit tests.

image

Two points:

@rcebulko
Copy link
Contributor Author

Thanks for looking into this. This is the second time we've had something fail tests but not builds; is this #34040 the same issue as before, or a separate one?

@rsimha
Copy link
Contributor

rsimha commented Apr 27, 2021

This is the second time we've had something fail tests but not builds;

In this case, the incorrect paths were all in test/unit/polyfills/, so it's expected that building the code (amp dist) would work, but not tests (amp unit). The tests should have failed, but didn't due to the reason described above.

is this #34040 the same issue as before, or a separate one?

Probably the same issue. #34040 fixes it at our end by throwing when zero tests are detected (this should never happen normally). @jridgewell Do you think we can get karma-esbuild to throw when it encounters transformation errors?

rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
…mpproject#34020)

* Move rest of polyfills unit tests to test/unit/polyfills

* Move polyfill stubs into polyfills/stubs and update imports

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

Successfully merging this pull request may close these issues.

4 participants