-
-
Notifications
You must be signed in to change notification settings - Fork 71
Temporary branch for testing CI failures #1569
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: develop
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Please upload reports for the commit 9086e61 to get more accurate results. Additional details and impacted files
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Just a quick test to check whether #1571 is enough to fix up the error (apparently, not) |
00b5f47
to
54243ab
Compare
Now that it isn't just me looking at this branch, I should make explicit: The tests added on this branch will never pass. They fail deliberately in order to force screenshots to reveal what the test users see. For anyone else following this, you can see the latest screenshots on the "Artifacts" tab of the CircleCI pages listed under "2 failing checks" on this page. @Oaphi Thanks for the fixes and for checking if they helped here. I notice that this pull request only shows 1 file changed. Is what happened that you merged in the changes from 1571 and then force pushed back to how this branch was before that merge? I noticed that the test failures on the CI still start with the nil post_type problem, but is that just because this branch no longer has your fixes? |
When the underlying problem(s) are fixed, I would expect the screenshots to change from showing zero posts to showing 7 posts in the Main category and 1 post in the Meta category (7 posts present in the fixtures, and 1 post added to Meta by standard_user during a test). |
I've merged |
I had forgotten that without my changes in #1548 there is a filter on the Main and Meta categories for standard_user, causing them to see zero posts. Therefore it is only basic_user who will see 7 posts in Main. The screenshot for the test |
Yup, I, merged my changes in, waited for the run to fail, then force-pushed to remove the merge commit (planned to leave it up to you to revert but decided that cleaning by myself is a better option) |
Thanks for confirming. Now that I've merged your changes back in I can see that the nil post_type problem seems to be fixed (thank you!) and the screenshots suggest a different problem. |
Running the tests locally does not result in the problem of standard_user already being signed in for the basic_user test. The screenshot shows basic_user signed in as intended, but now with 0 posts showing in Main (rather than the previous 7 posts). Previously:
Now:
|
Also it seems like system test 3.1 and 3.2 differ in that only the 3.1 test has the "already logged in" error displayed if I am not mistaken? |
Oh - thanks for checking. Previously Ruby 3.1 and 3.2 were only differing in how much of the nil post_type error was being displayed, and I didn't think to check both this time. Yes, 3.1 shows "You are already signed in" with standard_user signed in when that wasn't part of the basic_user test, while 3.2 shows the same as I'm seeing locally (where I'm running 3.1.6) - basic_user signed in with 0 posts in Main instead of the 7 present in the fixtures. |
I added a test to see what basic_user sees in Main, but this time with a log_out before attempting to log_in. This gives a screenshot of a plain white page (no header, no footer, completely blank). This happens locally (where previously basic_user could sign in but saw zero posts), on CI for 3.1 (where previously basic_user could not sign in due to standard_user being already signed in) and on CI for 3.2 (which previously looked identical to locally). |
This reverts commit 95c14ef.
After you added log_out on teardown, I still saw the blank white page for the test that tries to log_out before seeing what basic_user sees, for CI on both 3.1 and 3.2, and locally. I also saw "You are already signed in" for "what_does_standard_user_see_in_main" on both CI 3.1 and locally, but not for CI 3.2. I'm not sure if this is a real difference between versions or intermittent behaviour. |
The tests run in a random order each time, so if behaviour is inconsistent it suggests there is still state being carried over from one test to the next. |
There does seem to be state poisoning between tests, yeah. I went on a tangent to install FF in the container to be able to run system tests locally - now that I can test without waiting for the pipeline to run, it looks like the |
After doing some cursory testing, from what I can see, one of the issues is that preemptive It also doesn't look like "temporary test to analyse CI failures what does basic user see in main with log out first" starts with a poisoned state, at least with the latest commit (the visited page correctly shows that there is no signed in user). |
The screenshot for After adding a page visit before trying to log out, it now shows a logged out page (on CI 3.1, 3.2, and locally). The test fails with Also, the signed out page shows 7 posts (as expected) locally, but 0 posts on CI (3.1 and 3.2). |
There is also a new failure on CI 3.1 (not failing on CI 3.2 or locally). It's
I don't know what is causing it or why it only shows up on CI and only for 3.1. |
The older test |
Now that CircleCI is back up I have merged in |
That's actually reassuring - now the result is consistent. May it be that the seeds now apply properly so we need to adjust accordingly? |
Even the signed out test "what does basic user see in main with log out first" shows zero posts in main after basic_user fails to log in, so it does look like the posts are simply not present rather than being a user visibility problem. Yes I like the consistency. I don't know enough about how the fixtures work to understand why they aren't being picked up. I had previously thought that the fixtures were used before each individual test, but it now seems that's not the case and I don't know why. |
Not a real pull request.
This is just a way to run the CI system tests to try to understand why they are failing on #1548