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

Ruby 3.2.0 compatibility #170

Closed
wants to merge 4 commits into from
Closed

Ruby 3.2.0 compatibility #170

wants to merge 4 commits into from

Conversation

Yobilat
Copy link

@Yobilat Yobilat commented Dec 28, 2022

Changes:

  • Added compatibility for ruby 3.2.0
  • Tested with newer versions of puppeteer
  • Miscellaneous (rubocop, manual) fixes
  • Github actions update

Questions:

  • Do we need to test puppeteer as far as v1 given still suported nodejs versions?
    • If not can we clear up tests that depends on puppeteer version (puppeteer_version_on_or_after?)?
  • Some tests depends on external resources which are now failing - probably because heroxu axed free dynos. Any sugestions if/how to replace that?

@abrom
Copy link
Contributor

abrom commented Jan 9, 2023

Do we need to test puppeteer as far as v1 given still suported nodejs versions?
If not can we clear up tests that depends on puppeteer version (puppeteer_version_on_or_after?)?

No, likely safe to start dropping some of the older ones. Once an older version is dropped then it would be safe to remove the guard around any of the puppeteer_version_on_or_after tests (but the tests themselves shouldn't be removed!!?)

Some tests depends on external resources which are now failing - probably because heroxu axed free dynos. Any sugestions if/how to replace that?

Ahh.. yes. I'll need to fix that, thanks.

@Yobilat
Copy link
Author

Yobilat commented Jan 9, 2023

I see that cookie rederer is being replaced so I've removed older checks for puppetter versions(with assumption that "supported" versions are roughly those in updated GH actions).

@Yobilat Yobilat marked this pull request as ready for review January 9, 2023 09:16
@abrom
Copy link
Contributor

abrom commented Jan 9, 2023

yes, although there is another issue with a change to how newer versions of rspec deal with hash vs keyword arguments I still need to fix up 😬

@cpjmcquillan
Copy link

The rspec-mocks hash vs keyword arguments issue with Ruby 3.2 is resolved in 3.12.2, so you shouldn't have to change anything in your tests so long as rspec has been updated

@Yobilat
Copy link
Author

Yobilat commented Jan 9, 2023

I've already fixed tests regarding that.

@cpjmcquillan
Copy link

cpjmcquillan commented Jan 9, 2023

I've already fixed tests regarding that.

Sorry, I didn't mean to chime in and detract from the work happening here.

I only wanted to point out the issue described in rspec/rspec-mocks#1513 was resolved in rspec/rspec-mocks#1514, and as far as I know tests shouldn't need to be fixed as long as rspec-mocks has been updated to the latest version.


On another note, would it be possible to relax the required_ruby_version constraint?

I'm currently blocked from updating Ruby version by this requirement. I understand not wanting to support older versions of Ruby and only wanting to explicitly support certain versions at a given time, but this feels overly restrictive.

It also adds some pressure to maintainers when new versions are released that could be avoided.

How do you feel about adjusting the requirement to something like this instead?

spec.required_ruby_version = ">= 2.7.0"

Thanks to everyone that works on this gem, your work is appreciated ❤️

@abrom
Copy link
Contributor

abrom commented Jan 9, 2023

I've already fixed tests regarding that.

Sorry, I didn't mean to chime in and detract from the work happening here.

I only wanted to point out the issue described in rspec/rspec-mocks#1513 was resolved in rspec/rspec-mocks#1514, and as far as I know tests shouldn't need to be fixed as long as rspec-mocks has been updated to the latest version.

That's not the case. The tests run against the latest versions of rspec and rspec-mocks. See https://github.com/Studiosity/grover/actions/runs/3870089736/jobs/6596725310

On another note, would it be possible to relax the required_ruby_version constraint?

I'm currently blocked from updating Ruby version by this requirement. I understand not wanting to support older versions of Ruby and only wanting to explicitly support certain versions at a given time, but this feels overly restrictive.

It also adds some pressure to maintainers when new versions are released that could be avoided.

How do you feel about adjusting the requirement to something like this instead?

spec.required_ruby_version = ">= 2.7.0"

Thanks to everyone that works on this gem, your work is appreciated ❤️

Except then you have the situation where someone upgrades to the latest version.. gem installs fine, thus assumes everything must be fine. Except what if it isn't. If you have unbounded version checks you leave yourself very open to the unexpected/breakages/bugs/crashes/etc. It's why the project has the version matrix testing the code in CI.

Short term option.. fork the project, update the required version to your liking, use the fork in your project until grover + 3.2.x has been properly tested. 😄 But of course that goes with the caveat that it hasn't been properly tested.

@breisig
Copy link

breisig commented Jan 16, 2023

We REALLY need Ruby 3.2 support.

This was referenced Jan 20, 2023
@abrom
Copy link
Contributor

abrom commented Jan 20, 2023

FYI @Yobilat I've released Ruby 3.2.x grover support in v1.1.3

Thank you for your efforts in getting this through, although note I stripped parts of your PR out into separate PRs. It was trying to do a little too much all in one go, and I wasn't a fan of your matrix re-org.. it would have meant a full 3 (node versions) X 6 (puppeteer versions) + 3 static builds per run.

@abrom abrom closed this Jan 20, 2023
@Aesthetikx
Copy link

Great work! Thanks @abrom

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.

None yet

5 participants