Prune Comment System Tests for Faster CI

[noi5e]

This should have happened a long time ago...

This is all up for debate, but I'm pruning the comment system tests so that System Test CI doesn't take 20 minutes. Locally, when I run this PR, the comment system tests take 4 minutes. Big improvement!

I know the structure of comment_test.rb is confusing, so I'll break it down here. The system tests are divided into 3 parts:

1. Posting comments, posting comment replies, deleting comments, and editing comments. These four basic CRUD-type functions are tested for the Rails commenting system, and then run a second time on the React commenting system.
2. The vast majority of comment tests are run on research notes.
3. A select few tests are run on research notes, AND wikis, AND questions.

This PR does the following:

• Comments out tests for cross-wiring image upload bugs (see Drag and drop feature of comment editor does not work properly when editing a comment #8670 for more context). The comment system rewrite reduces the likelihood of these bugs occurring. But if they ever come back, we can un-comment these again.
• Moves comment moderation tests from Part 3 to Part 2, so they're only run on research notes, and not the other two page types.
• Moves image upload and rich-text tests from Part 3 to Part 2 as well.

---

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

[gitpod-io]

[codecov]

Codecov Report

Merging #9752 (9e9fa0c) into main (f41f8e7) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 9e9fa0c differs from pull request most recent head d38986b. Consider uploading reports for the commit d38986b to get more accurate results

@@            Coverage Diff             @@
##             main    #9752      +/-   ##
==========================================
+ Coverage   82.13%   82.26%   +0.12%     
==========================================
  Files          98       98              
  Lines        5968     5966       -2     
==========================================
+ Hits         4902     4908       +6     
+ Misses       1066     1058       -8     

Impacted Files	Coverage Δ
app/services/search_service.rb	95.00% <100.00%> (-0.10%)	⬇️
app/api/srch/search.rb	71.33% <0.00%> (+4.45%)	⬆️
app/services/execute_search.rb	94.44% <0.00%> (+5.55%)	⬆️

[noi5e]

Down to 12 minutes in this PR, compared to 24 in the other one I have open.

[RuthNjeri]

This is pretty neat @noi5e...my question is about

Comments out tests for cross-wiring image upload bugs (see #8670 for more context). The comment system rewrite reduces the likelihood of these bugs occurring. But if they ever come back, we can un-comment these again.

In my opinion, those tests should not be commented out. In the event they fail, it would signal to us that the bug has re-occured. It is better for us to find out from the tests first than the user...let me know your thoughts...

[jywarren]

Hi, this is a great conversation and an impressive speed-up!

Just so we know all the possibilities, is it at all possible to parallelize these tests, for example to run some of the system tests concurrently? Either by splitting up the tests into 2 suites, or by using some switch to run half the tests in one job and half in another? I totally hear @RuthNjeri's input on keeping them online, i just wonder if we can have both?

[noi5e]

@RuthNjeri and @jywarren thanks for the reviews and thoughts. I definitely see the importance of keeping the tests just in case they pop up again in the future.

@jywarren Do you have any links for documentation on how to run these tests in parallel? Is it something like what's described here? Or would it be something that's done in Capybara (or whatever our testing suite is exactly, still not sure 😅)

[jywarren]

Yes exactly, in different jobs most likely in GitHub actions. This is relatively low priority though, and I worry that you're taking on more than your scope, so please only take this on if it brings you joy!

[jywarren]

What if we used this PR to try moving the commented-out tests into a separate job? Does it work to simply make a folder and file called /commenting/full_stack_comment_tests.rb and then invoke it via rails test test/commenting/full_stack_comment_tests.rb as described in https://github.com/publiclab/plots2/blob/main/doc/TESTING.md#running-just-one-test ?

Then we could run it with a copy of the system tests job, changing only this last line:

plots2/.github/workflows/tests.yml

Line 130 in cf6f681

bundle exec rails test:system

to:

rails test test/commenting/full_stack_comment_tests.rb

[noi5e]

What if we used this PR to try moving the commented-out tests into a separate job? Does it work to simply make a folder and file called /commenting/full_stack_comment_tests.rb and then invoke it via rails test test/commenting/full_stack_comment_tests.rb as described in https://github.com/publiclab/plots2/blob/main/doc/TESTING.md#running-just-one-test ?

I'll try experimenting with this during this week. Just git rebased!

[jywarren]

Just a note that due to our codecov thresholds config, we'll have to change after_n_builds to 5 in two spots in this file, to merge this!

plots2/codecov.yml

Lines 6 to 30 in a65efa6

	after_n_builds: 4
	require_changes: true # if true: only post the comment if coverage changes
	require_base: yes # [yes :: must have a base report to post]
	require_head: yes # [yes :: must have a head report to post]
	branches: null # branch names that can post comment
	coverage:
	precision: 2
	status:
	project:
	default:
	threshold: 3%
	patch:
	default:
	threshold: 50%
	ignore:
	- app/assets/stylesheets/
	codecov:
	# Avoid "Missing base report" due to committing CHANGES.rst with "[CI skip]"
	# https://github.com/codecov/support/issues/363
	# https://docs.codecov.io/v4.3.6/docs/comparing-commits
	allow_coverage_offsets: true
	notify:
	after_n_builds: 4

Looks good though @noi5e !!

[noi5e]

Just pushed a commit increasing after_n_builds to 5, from 4.

[noi5e]

The new comment-system-tests workflow won't start for some reason, here's the most recent error. I got this same error even before I made the last commit to change after_n_builds:

[noi5e]

I changed the bundle exec rails test:comment_system_tests command to bundle exec rails test test/comment_system_tests. I think this will still fail.

My guess is that for whatever reason Rails needs extra instruction in order to run tests in a custom directory (that's not unit, functional, integration, or system). But how to get it to do that? Something like this from StackOverflow?

[noi5e]

Again, the test doesn't start, screenshot of the most recent failed test build:

[jywarren]

Hm, i think the "unexpected end" was from a mistaken extra "end" on line 480, maybe? I tried removing it and was able to run rails test test/comment_system_tests in GitPod (although without chromedriver installed there the tests didn't actually run). Let's see if that gets them to run in Github Actions?

[jywarren]

🎉 🎉 🎉

[jywarren]

Cool but whoa -- tests / integration-tests (pull_request) Successful in 26m

vs in #9958 --

Should we re-trigger them to try to get better times?

[jywarren]

Re-running to see...

[jywarren]

Wow whats going on with integration tests?

[jywarren]

Running again. Could it have to do with time of day or something? Or a GitHub Actions slowdown? The integration tests themselves really did clock 1390 seconds or 23 minutes.

[jywarren]

Hmm. It's pretty consistent. What do you think is going on here?

[jywarren]

OK - speed issues seem to be across the board. Here's an unrelated recent PR: #9965

Huh, yes, they're all this way: https://github.com/publiclab/plots2/actions/runs/1072626662

[jywarren]

This, 9 days ago affected integration tests for the first time in a few months: https://github.com/publiclab/plots2/pull/9878/files

[jywarren]

But it was only a whitespace change, actually. So it could be a change to our actual application code that has caused integration tests to slow significantly.

[jywarren]

This, 2 days ago, ran fast: https://github.com/publiclab/plots2/actions/runs/1071468860

Hmm.

[jywarren]

OK, the change was between:

change webpacker.yml to see public/lib
#9959 merged 2 days ago

Update merge-pr-sentry-release.yml to change sentry environment to "staging"
#9956 merged 2 days ago

So it was likely the change to webpacker.yml in response to the tocbot fix:

-  additional_paths: []
+  additional_paths: ['public/lib']

Since we found that this didn't actually solve the problem, and merged #9918 instead, let's just revert #9959 for now to speed things back up.

[jywarren]

Now watching #9966 for faster load times. Let's merge that and then rebase this on top of it to get a final speed check (assuming this works).

[jywarren]

That worked. They failed due to an intermittent issue, but went back to their original load times:

[jywarren]

Now rebasing over #9966 having just merged it. Fingers X and we should be able to merge this then!

[codeclimate]

Code Climate has analyzed commit 2001d46 and detected 0 issues on this pull request.

View more on Code Climate.

[jywarren]

Let's also update the branch protection rules to include this additional set "comment-system-tests" once we merge this!

[jywarren]

Yay!!!

I'll merge this now!!!

[noi5e]

Nice! Thanks for the assist on this @jywarren