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

Just a test of current CI: empty commit to test CI #308

Closed
wants to merge 2 commits into from
Closed

Conversation

jrochkind
Copy link
Member

No description provided.

@jrochkind
Copy link
Member Author

OK we have reproduced test failure on main with no changes! Annoying! I wonder what changed since last succesful test runs.

The error is somehow only in bl ~> 8.0 / rails 7.2.1 / rb 3.3 / importmap-rails, propshaft, but that's an important one!

Error seems to be on all browser tests, a browser console error (hooray for browser console errors included in test output) that says:

SEVERE: http://127.0.0.1:36079/assets/application-9dd3ef3f.js 6:0 Uncaught SyntaxError: Identifier 'githubAutoCompleteElement' has already been declared"

I wonder if some JS code is getting run twice? I will try to reproduce locally and diagnose and fix.

@jrochkind
Copy link
Member Author

have succesfully reproduced locally. not yet debugged or diagnosed, tricky to figure out what's up with propshaft.

@jrochkind
Copy link
Member Author

OK in generated .internal_test_app application.js there is indeed a duplicate:

import githubAutoCompleteElement from "@github/auto-complete-element";
import githubAutoCompleteElement from "@github/auto-complete-element"

So have to figure out what's generating that and fix it! Maybe some version of BL 7.x started adding something not previously added.

@jrochkind
Copy link
Member Author

Based on run history, seems build worked for this cell with BL 8.6.1, but broke with BL 8.7.0. (it actually failed in a PR we merged anyway recently, not sure why we did that!).

Haven't yet reproduced locally.

@jrochkind
Copy link
Member Author

OK, I think our generator for BL 8 fixup is no longer necessary in BL 8.7.0 and in fact causes problems. Good that BL 8.7.0 changed to make our extra generator fixup not necessary, but, phew!

Will PR separately.

@seanaery
Copy link
Collaborator

seanaery commented Jan 7, 2025

@jrochkind Thanks so much for tracking all that down. The test matrix all passed in the original PR #305 which was issued Dec 3, so that would've been using BL 8.6.1. Once the PR was merged to main on Dec 6, BL8.7.0 had been recently cut (Dec 4), so it adds up that a change in BL 8.7.0 is the culprit.

jrochkind added a commit that referenced this pull request Jan 7, 2025
And actually causes problems in 8.7.0, trying to do something that was already done. See #308
@jrochkind
Copy link
Member Author

Fixed in #309

@jrochkind jrochkind closed this Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants