-
Notifications
You must be signed in to change notification settings - Fork 160
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
Migrate test suite to switchblade #977
Conversation
robdimsdale
commented
Jul 25, 2024
- delete low-value tests.
942d784
to
7b1f50a
Compare
- delete low-value tests.
7b1f50a
to
9314709
Compare
It("works with old version of bundler 2", func() { | ||
app = cutlass.New(Fixtures("bundler_2")) | ||
PushAppAndConfirm(app) | ||
Expect(app.Stdout.String()).To(MatchRegexp(`Your Gemfile.lock was bundled with bundler 2\.\d+\.\d+, which is incompatible with the current bundler version \(2\.\d+\.\d+\)`)) | ||
Expect(app.Stdout.String()).To(ContainSubstring(`Deleting "Bundled With" from the Gemfile.lock`)) | ||
Expect(app.GetBody("/")).To(ContainSubstring("Hello world!")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we support building apps with EOS Bundler versions, this feels like something we should test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this test was written it wasn't really designed to be an EOL test, bundler 2 was just new. The log statements were added later.
Bundler 2 isn't EOL, and I'm not aware of any reason why we should care about testing that apps bundled with older bundlers should work with the buildpack. My understanding is that bundler handles this. It's not a buildpack behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we actually delete the line in the Gemfile.lock file that causes issues here -
ruby-buildpack/src/ruby/supply/supply.go
Lines 842 to 843 in 40bdf71
match := regexp.MustCompile(`BUNDLED WITH\s+(\w|\.|-)+\n`) | |
output := match.ReplaceAll(file, []byte("")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I think that's fair feedback. I'd be happy to include that.
src/ruby/integration/deploy_an_app_with_custom_gemfile_bad_dummy_gemfile_test.go
Show resolved
Hide resolved
Expect(app).To(HaveUnchangedAppdir("BuildDir Checksum Before Supply", "BuildDir Checksum After Supply")) | ||
|
||
Expect(app.GetBody("/")).To(ContainSubstring("Successfully required execjs")) | ||
Expect(app.Stdout.String()).ToNot(ContainSubstring("ExecJS::RuntimeUnavailable")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not a case we care to test for anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, there's nothing special about execjs that isn't covered by other apps with javascript (rails6_sprockets
, rails7
, etc). It's just a library that happens to have a dependency on javascript.
This test was added 9 years ago. There's no real context in the tracker story other than "using the latest version of nodejs".
So I wonder if this test was just the one example they used to drive out nodejs support. But we have that covered by other tests.
It's hard to comment because my GH UI keeps freezing. My main point of review is understanding the reasons we deleted some tests with no replacement. I understand some might not be good tests, or might be out of date. Some explanation of the more controversial removals might be useful |
😬
Sure. But I don't know what you consider controversial :) I can go through and explain each test removal |
@robdimsdale I can try to save you some time and make a list of the ones im wondering about |
@robdimsdale I think we covered the main items I was concerned about, let's get this merged in |