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

Fix System Tests in CI environment #538

Merged
merged 3 commits into from Dec 14, 2023

Conversation

seanpdoyle
Copy link
Contributor

@seanpdoyle seanpdoyle commented Dec 7, 2023

Add rails@7.1 to the CI Test Matrix

As part of reading from the RAILS_VERSION environment set in CI,
update the test/dummy/config/application.rb to use that environment
variable when loading defaults.

Fix a test failure when rails@7.1 by delegating Message#to_s to
Message#content so that tests that broadcast messages without text
continue to pass.

Also remove an entry with ruby: "head", since we aren't specifying
head in the ruby: key.

Fix System Tests in CI environment

Resolve breaking changes introduced in @hotwired/turbo that were later
fixed in @hotwired/turbo#4f334da (and further resolved in
@hotwired/turbo#7a7c6e2).

CI is failing because those changes are not available as part of a beta
release. Despite the fact that these changes wouldn't be necessary if
that release were available, this resolves the broken build pipeline
more immediately.

Assign window.Turbo in @hotwired/turbo-rails/index

With the changes made in the prior commit to pass CI merged, the fact
that Turbo and Turbo Rails somehow have differing references to Turbo
and window.Turbo.

This commit reverts the test harness change that sets window.Turbo,
and instead assigns that value as part of the
@hotwired/turbo-rails/index module:

-import { Turbo } from "@hotwired/turbo-rails"
-
-window.Turbo = Turbo
+import "@hotwired/turbo-rails"

@seanpdoyle seanpdoyle force-pushed the import-stream-actions branch 2 times, most recently from 2a4640e to d4abcd8 Compare December 7, 2023 18:15
@seanpdoyle seanpdoyle requested a review from zzak December 7, 2023 19:16
@seanpdoyle seanpdoyle force-pushed the import-stream-actions branch 4 times, most recently from 1950a13 to 50e4853 Compare December 7, 2023 19:20
Copy link
Contributor

@zzak zzak left a comment

Choose a reason for hiding this comment

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

LGTM, but maybe want to squash the commits.

Tested locally and it fixes the system tests. 👍

As part of reading from the `RAILS_VERSION` environment set in CI,
update the `test/dummy/config/application.rb` to use that environment
variable when loading defaults.

Fix a test failure when `rails@7.1` by delegating `Message#to_s` to
`Message#content` so that tests that broadcast messages without text
continue to pass.

Also remove an entry with `ruby: "head"`, since we aren't specifying
`head` in the `ruby:` key.
Resolve breaking changes introduced in `@hotwired/turbo` that were later
fixed in [@hotwired/turbo#4f334da][] (and further resolved in
[@hotwired/turbo#7a7c6e2][]).

CI is failing because those changes are not available as part of a beta
release. Despite the fact that these changes wouldn't be necessary if
that release were available, this resolves the broken build pipeline
more immediately.

[@hotwired/turbo#4f334da]: hotwired/turbo@4f334da
[@hotwired/turbo#7a7c6e2]: hotwired/turbo@7a7c6e2
@MSP-Greg
Copy link
Contributor

MSP-Greg commented Dec 11, 2023

@seanpdoyle

Thanks for fixing this.

Also remove an entry with ruby: "head", since we aren't specifying head in the ruby: key.

Values used in 'include' do not need to be in the normal matrix. For instance, Ruby Windows has four head builds available in GitHub Actions, head, ucrt, mingw, and mswin. ruby/openssl & puma/puma both run mswin jobs in their 'include' section.

EDIT: I just ran CI in my fork with the 'include' ruby head line added back in, passed here.

With the changes made in the prior commit to pass CI merged, the fact
that Turbo and Turbo Rails somehow have differing references to `Turbo`
and `window.Turbo`.

This commit reverts the test harness change that sets `window.Turbo`,
and instead assigns that value as part of the
`@hotwired/turbo-rails/index` module:

```diff
-import { Turbo } from "@hotwired/turbo-rails"
-
-window.Turbo = Turbo
+import "@hotwired/turbo-rails"
```
@seanpdoyle
Copy link
Contributor Author

@jorgemanrubia are you able to review these changes?

Copy link
Member

@jorgemanrubia jorgemanrubia left a comment

Choose a reason for hiding this comment

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

Thank you so much @seanpdoyle

@jorgemanrubia jorgemanrubia merged commit 75b25b5 into hotwired:main Dec 14, 2023
15 checks passed
@seanpdoyle seanpdoyle deleted the import-stream-actions branch December 14, 2023 17:21
@MSP-Greg
Copy link
Contributor

Thanks. Puma's turbo-rails workflow, which uses both Puma head and turbo-rails head is now passing.
https://github.com/puma/puma/actions/runs/7212424085

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

Successfully merging this pull request may close these issues.

None yet

4 participants