Add fullstack tag for system specs

[mjankowski]

This is sort of a followup on a bunch of previous PRs:

• Add JS console errors check #28682
• Extract capybara config and improve headless_chrome driver config #28681
• Configure streaming spec via rspec tags #27932
• Capybara config update #27732

As noted in comments across those, I had a few spec coverage goals that are blocked by the current configuration of system specs:

• Continue to modernize some of our controller coverage, moving portions of it into request specs, and other portions into system specs (Likely driven by rack_test for a good chunk of the settings/admin ones); leaving behind only the things which are narrowly about the controller as a ruby class. I suspect this will take a while but will be good to go one chunk at a time, and will hopefully lead to some spec LOC reduction, but improvement in clarity and coverage.
• Add more full-stack coverage that exercises the react app -- this is a massive gap right now

Previous efforts have been rejected for various naming/terminology/organizational reasons. This one takes a slightly different approach to hopefully handle the issues from the prior ones. Summary of changes:

• Move all the system specs which were previously in spec/features/ to spec/system/. This is very much a naming-only move to align with where current rspec/rails versions put things (misc reading: 1, 2, 3)
• Move the system specs which were previously the only occupants of spec/system/ into spec/system/fullstack/ as a way of isolating them from the others (to enable next bullet...)
• Update rspec config to apply the :fullstack tag to all spec files in that directory.
• Update the capybara configuration to use rack_test as the default driver for system specs; and to use the configured Capybara.javascript_driver (currently headless chrome) for examples with that :fullstack tag.
• Update the configuration which was skipping all of spec/system by default (run in CI only) to still skip them, but to do it based on the rspec tag instead of an env var.
• Misc other config changes (CI command, rake task, streaming manager, js errors check, etc) all along those lines as well

Curious to get feedback on this direction, will leave a few inline comments re: future ideas.

[ClearlyClaire]

Overall, I think this is a sensible approach. I'm curious if the extra setup for fullstack tests would cause issues for non-fullstack tests. i.e. can you do bundle exec rspec --tag fullstack and have it run all the tests without issues?

There currently are test failures on the CI, but I haven't investigated them.

[mjankowski]

Overall, I think this is a sensible approach. I'm curious if the extra setup for fullstack tests would cause issues for non-fullstack tests. i.e. can you do bundle exec rspec --tag fullstack and have it run all the tests without issues?

Good question ... we'd have to look at both using the JS-browser for the previous system specs that use rack_test and see if they are ok under selenium (they were NOT on my quick initial checks in a previous PR); and we'd also have to look at whether having the streaming server running and connected in the background impacts those or any other specs as well. I haven't thoroughly looked at this, other than that initial check.

There currently are test failures on the CI, but I haven't investigated them.

Yeah there's definitely some issue here. It's a little hard because we do have those intermittent failures across the selenium-driven system specs already ... but I'm also seeing these as intermittent local failures more so in this PR branch than regularly, and with different failures, so there's probably something going on from this branch. I'll work on that a bit.

Separately, for me locally, the oauth spec specifically always fails entirely. Not sure if there's a mac/linux issue where it fails for me and passes CI, or some other env/config thing I've got going where I don't realize the difference.

[mjankowski]

Think I worked out the flaky specs aspect ... I had forgotten to change the db cleaner config in a place it needed to be changed, and changed it in a place it did not need changing. Adjusted that, things passing now. Still looking at other stuff.

[ClearlyClaire]

I do appreciate the switch from an environment variable to a tag, but taking a step back, I find a sub-directory (spec/system/fullstack) having special behavior slightly weird, and I think the previous behavior of having this dependant on top-level directories made more sense.

Are you aware of any established convention around that in other Rails project?

[mjankowski]

I do appreciate the switch from an environment variable to a tag, but taking a step back, I find a sub-directory (spec/system/fullstack) having special behavior slightly weird, and I think the previous behavior of having this dependant on top-level directories made more sense.

I also think it's weird, but added it here based on prior guidance.

Are you aware of any established convention around that in other Rails project?

In 99% of every (recent-ish) Rails project I've looked at, the convention is to put all the system specs into spec/system/ and use rspec tags to guide which configured capybara browser drives those examples, usually on a per-file basis. This is usually done with having rack_test as the default driver, and then configuring a :js or :javascript tag to use whatever the configured selenium JS-enabled browser is. I tried this in a previous PR and it was rejected.

The only special case we have on top of that is the spinning up/down of the streaming server via that manager class.

[ClearlyClaire]

Trying to sum up my thoughts about all this:

• I do think using a disabled-by-default tag rather than the weird mix from before is an improvement
• I think having such heavy specs in a separate directory does make sense, but having them in a sub-directory of spec/system feels a bit weird
• I guess we could have one tag for selenium and one tag for the streaming server (e.g. the admin system spec that test JS do not need the streaming server), but maybe that's overkill?

[ClearlyClaire]

Also, on an adjacent topic, I only just now noticed the Webpacker.compile call. Is that really something we need? Regular tests also require precompiled assets, so what's the difference here?

[mjankowski]

• I do think using a disabled-by-default tag rather than the weird mix from before is an improvement

Great, agreed.

• I think having such heavy specs in a separate directory does make sense, but having them in a sub-directory of spec/system feels a bit weird
• I guess we could have one tag for selenium and one tag for the streaming server (e.g. the admin system spec that test JS do not need the streaming server), but maybe that's overkill?

I think what you described is the most typical and intended way to do this ... use tags at the appropriate level (whole file, just a context, just one example, etc) to describe the features you want on/off/changed/whatever. We do this already for the paperclip processing and sidekiq behavior; so also doing it for the "I need a streaming server running" and "I need to use a JS browser" use cases feel normal to me. I tried this in a previous PR that was rejected.

With the appropriate tagging in place, you can then configure what should run by default (while preserving ability to run full) and what should only run on CI and so on.

Part of what I think might feel odd here is that since we currently have this hard separation by directory breaking that seems weird ... but in some hypothetical future when we migrate some of the current controller specs to become system specs, and ideally add many more new ones for some of the JS-requiring features it will not seem weird.

My own side note - I think it makes sense to use dirs for things like "admin", kind of like we have in the api request specs and in the controller specs now ... basically have it mirror either the actual primary code organization or some app-level concept; rather than for feature enabled/disable (which like you noted, is better done with tags).

[ClearlyClaire]

Let's say we change current spec/system tests to be tagged with :js, and some of them with :streaming, we would disable js and streaming by default. How would we run only tests that have either of these tags in the E2E tasks?

[mjankowski]

Putting aside whatever env vars or other config, I think the CI command would just be like bin/rspec --tag js --tag streaming - which would run all the examples with either or both tags?

[ClearlyClaire]

Putting aside whatever env vars or other config, I think the CI command would just be like bin/rspec --tag js --tag streaming - which would run all the examples with either or both tags?

But it would also run every other test, right?

[mjankowski]

Putting aside whatever env vars or other config, I think the CI command would just be like bin/rspec --tag js --tag streaming - which would run all the examples with either or both tags?

But it would also run every other test, right?

If I understand the question, then yes it would still run everything on CI. What I was envisioning on CI was to preserve the current approach:

• Run plain bin/rspec with no args/tags/options for the main test step. Would run everything except what we configure to exclulde by default (js and streaming in our hypothetical, in addition to already-excluded search)
• Run bin/rspec --tag js --tag streaming for the test-e2e step, which would run anything tagged with one/both of those, and which would need to spin up the streaming server and use headless chrome.
• Leace the search one as-is -- this already uses a tag approach for exclusion and the search data manager.

For local dev, you'd have the same default behavior of excluding the defaults, but could opt in with the tags if desired.

If that was not what you meant ... then I don't know what you meant.

[ClearlyClaire]

Sorry, I misunderstood how --tag worked, bin/rspec --tag js --tag streaming would do exactly what we want (running only tests that involve either tag).

[mjankowski]

Should I reformulate this into one that includes both the file dir consolidation and the tag approach into another PR and we can review that?

[ClearlyClaire]

What do you mean by the file dir consolidation?

[mjankowski]

What do you mean by the file dir consolidation?

Moving the system specs which are currently in spec/features/ into spec/system/ (as done in this PR).

I could probably start a branch from this PR's branch, and keep the file moves, but replace the fullstack dir/tag approach with a pure tag approach using js/streaming, and see how that looks?

[ClearlyClaire]

Yeah, sounds like a plan to me!

[mjankowski]

Added #30206 -- I like the approach done there (new PR) better, but will leave this open until both can be reviewed.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[mjankowski]

Closing in favor of #30206