-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Introduce acceptance testing #2830
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
base: main
Are you sure you want to change the base?
Conversation
This is what's been keeping me busy the last few months. I wanted to put together a pattern for Administrate, where I've felt the need most keenly, but for other places too. I don't expect this to be merged ahead of us releasing v1, but getting here so far has been useful for testing. Remaining jobs:
Expected tests, broadly:
If anyone can think of another scenario which should be tested for, I'd love to hear it. |
gem "formulaic" | ||
gem "jet_black" | ||
gem "launchy" | ||
gem "sack_race", github: "nickcharlton/sack_race" |
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.
Before merging, I'll cut the full version of v1.0.0. It's at v1.0.0.rc1 (because it's been working well here so far).
end | ||
|
||
def app | ||
@app ||= RailsProcess.new(session) |
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.
I wasn't sure about the naming of RailsProcess
, is there a better one?
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.
How about TestAppProcess
? I feel that RailsProcess
may make it sound like something in the framework. Instead we want to convey that this is the process for the app we are testing against.
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.
Oh yeah that's much nicer.
expect(page).to have_content("Post was successfully created.") | ||
expect(page).to have_content("Show Post #1") | ||
|
||
app.stop |
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.
Something I didn't figure out yet: What if the test fails? We don't clean up the process.
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.
@nickcharlton Is it possible to add a simple rescue
block to catch the exception and clean up processes there?
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.
Yeah, it might be. I started looking into the RSpec hooks to see what the most correct way to do this would be but I couldn't figure it out.
The trouble with a simple rescue
is having to do it everywhere or have a bad time and that doesn't sit well with me.
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.
There is one possible approach called around
hook that will probably fit your needs in this test case.
This is great! |
Yes! That's what I'm thinking the eventual direction of this can be. Once we've good satisfactory coverage end-to-end of creating different applications in the scenarios I listed above and it's running in CI, added these in should be relatively easy to do. |
|
||
def start_and_wait_for_ready | ||
Bundler.with_unbundled_env do | ||
process.add_handler(:ready, "Use Ctrl-C to stop\n") |
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.
I saw this line in the sack_race readme and couldn't understand what it did. Now I see: it waits for the string to appear in stdout, triggering the given event.
Perhaps add a line to that effect to the readme? 😄
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.
Hah! Can you think of a better name? I might've gone a bit too generic when I was thinking it through.
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.
Hm... Perhaps set_event_on_stdout
? Looking at the code I see this is about stdout only, so I guess let's call it explicitly rather than set_event_on_output
which was my first thought.
Or perhaps add_stdout_handler
?
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.
Hm, I think I'm a fan of add_stdout_handler
. But then I'm not sure if the wait_for_handler
method also needs changing?
Or, does it suggest that there could also be a add_stderr_handler
? Which is a nice idea itself.
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.
I think set_event_on_stdout
and wait_for_event
pair well.
The word "handler" would be ok, except for the pairing with "wait for handler", which is not something I would normally expect. A handler handles, and I'm not sure it's something to wait for. I think to "wait for event" is more common.
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.
Oh yes, that's nice! Thanks!
end | ||
|
||
def app | ||
@app ||= RailsProcess.new(session) |
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.
How about TestAppProcess
? I feel that RailsProcess
may make it sound like something in the framework. Instead we want to convey that this is the process for the app we are testing against.
This is the culmination of several months work, and at least one side quest, in laying down the foundations of a test suite which will run Administrate completely from scratch. For each scenario, we create a new Rails application in a clean environment, install Administrate and run the generators. We can then test against this fresh application. Whilst we seem to have had a good track record when it comes to past and future compatibility, the hope is that by having such an outside perspective focused test suite, this will provide much broader confidence, especially where different frontend configurations are concerned. We'll also be able to test speculatively, such as against the `main` branch of Rails which isn't something we could've done with confidence in the past with so many different components involved. To do this, we introduce the `jet_black` gem, which helps with running "black box" tests for command line tools, and `sack_race`, which helps run and manage processes inside the test suite. To run the tests themselves, we use Capybara like we do elsewhere, only pointed against the Rails project we created as part of the test. It's not expected that we'd re-use our existing Capybara tests here, but rather create new ones for each scenario. As these tests are always going to be slow, we don't run these as usual when running `rspec`, so they excluded. We'll run them in GitHub Actions as standalone jobs. Over time, it's expected that we'd create acceptance tests for all supported configurations, covering different frontend tools, to projects with complex model configurations and beyond. https://github.com/odlp/jet_black https://github.com/nickcharlton/sack_race
f160784
to
c5356a3
Compare
This is the culmination of several months work, and at least one side quest, in laying down the foundations of a test suite which will run Administrate completely from scratch. For each scenario, we create a new Rails application in a clean environment, install Administrate and run the generators. We can then test against this fresh application.
Whilst we seem to have had a good track record when it comes to past and future compatibility, the hope is that by having such an outside perspective focused test suite, this will provide much broader confidence, especially where different frontend configurations are concerned. We'll also be able to test speculatively, such as against the
main
branch of Rails which isn't something we could've done with confidence in the past with so many different components involved.To do this, we introduce the
jet_black
gem, which helps with running "black box" tests for command line tools, andsack_race
, which helps run and manage processes inside the test suite.To run the tests themselves, we use Capybara like we do elsewhere, only pointed against the Rails project we created as part of the test. It's not expected that we'd re-use our existing Capybara tests here, but rather create new ones for each scenario.
As these tests are always going to be slow, we don't run these as usual when running
rspec
, so they excluded. We'll run them in GitHub Actions as standalone jobs.Over time, it's expected that we'd create acceptance tests for all supported configurations, covering different frontend tools, to projects with complex model configurations and beyond.
https://github.com/odlp/jet_black
https://github.com/nickcharlton/sack_race