Skip to content
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

Fix test workflow, bis #4252

Merged
merged 27 commits into from
Feb 19, 2022
Merged

Fix test workflow, bis #4252

merged 27 commits into from
Feb 19, 2022

Conversation

jgonggrijp
Copy link
Collaborator

This is a continuation of @ogonkov's work at #4251, which I'm personally pushing directly to the main repo in order to rule out any effects from permissions. I also wanted to try a couple of things. If I end up merging this branch, #4251 will automatically be recognized as merged as well.

ogonkov and others added 16 commits January 21, 2022 21:28
If I understand the documentation correctly, it is not strictly
necessary that these variables be read from the workflow file; they
may also be read directly by the target processes.
In Node 6, running Karma gave the following warning:

Error during loading "/home/runner/work/backbone/backbone/node_modules/karma-sauce-launcher" plugin:
  Unexpected token function

I had no such error running karma after installing karma-sauce-launcher
locally in Node 14, so this upgrade might fix some things.

We are not exactly in the business of supporting old Node versions, so
this should be relatively painless.
I mostly copied this from the current config in Underscore.
My account currently allows only two concurrent runs, so let's rule
out that this is biting us.
I chose this datacenter when creating my account. Maybe it matters.
I tried a couple of different OS version, and this one was the first
where Firefox 11 seems to connect correctly at least some of the time.

(To clarify: the tests were passing in Firefox 11 on all OS versions I
tried, but in most cases a connection error prevented Karma from
knowing this. I only know because I saw the reports in the Sauce web
interface.)
@jgonggrijp jgonggrijp force-pushed the patch-1 branch 2 times, most recently from 8f9af60 to 91b190b Compare January 22, 2022 03:06
@jgonggrijp
Copy link
Collaborator Author

@ogonkov @paulfalgout I got the Sauce Labs tests to run, mostly reliably, with the exception of Firefox 11. See the details of the above (failed) workflow run.

If you agree with the logic changes you see here, we can merge this. However, we need to make a decision about the lowest Firefox version.

The tests do pass in Firefox 11, as you can see on Sauce Labs: https://app.eu-central-1.saucelabs.com/tests/08d1010d4bea46dda25b29f2a6a9a698. However, something is going wrong in the connection with Karma. I tried this on a couple of OS versions and I had a single attempt on Windows 10 where the connection suddenly worked (run 53). So far, I haven't been able to reproduce this, but this is the reason why the karma.conf-sauce.js currently lists Firefox 11 with Windows 10.

I also tried Firefox 18, 25 and 32, but they all behaved similarly flaky, although versions 18 and 32 eventually succeeded on second attempts. See run 55.

I see three possible courses of action:

  • Keep Firefox 11 and accept that it will fail on most runs.
  • Drop Firefox 11 without replacement, making 40 the oldest version. This would mean that we are only testing relatively recent versions of Firefox.
  • Find a version between 11 and 40 with a better tradeoff between reliability and age. This can be done with a "quadrisect" (cf git bisect) where 18-25-32 was the first recursive step.

Please let me know what you think!

karma.conf-sauce.js Outdated Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
karma.conf-sauce.js Show resolved Hide resolved
jgonggrijp added a commit that referenced this pull request Jan 22, 2022
As suggested by @ogonkov in review to #4252.

This partly reverts commit 453bdf5.
As suggested by @ogonkov in review to #4252.

This partly reverts commit 453bdf5.
@ogonkov
Copy link
Contributor

ogonkov commented Jan 22, 2022

Can you run sause labs config with config.LOG_DEBUG? May be we get more info about what exactly the problem is

@jgonggrijp
Copy link
Collaborator Author

That was a good suggestion. The visible difference between FF11 and the other browsers was that FF11 was creating additional socket connections and then immediately closing them again. I tried increasing Karma's browserDisconnectTimeout, but that only seems to lead to accumulation of even more sockets.

Haven't been able to find a solution for "Response has empty body" or "Request failed with status 204" yet. Still searching the web.

@ogonkov
Copy link
Contributor

ogonkov commented Jan 22, 2022

Some requests (which one?) has been responded with empty body, that was unexpected for client. There also warn about old client. May be it is something related to old client, used by plugin?

@jgonggrijp
Copy link
Collaborator Author

Yes, good suggestion. I was just considering to upgrade the devDependencies in case that's playing a role. Will do that now.

@jgonggrijp
Copy link
Collaborator Author

I'm stuck again because Sauce Labs hasn't converted my trial account to Open Sauce yet and my trial has run out of testing minutes. To be continued.

@jgonggrijp
Copy link
Collaborator Author

Update: I haven't heard back from Sauce Labs since January 17. I just made a third attempt at reestablishing contact, using a different email address.

@jgonggrijp
Copy link
Collaborator Author

Update again: they finally enabled Open Sauce, so we can run browser tests again. I will resume the debugging labour soon.

- run: ./node_modules/.bin/karma start karma.conf-sauce.js
- name: Install dependencies
run: |
npm install --no-audit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want npm ci here, since we had package lock

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I will have a look at the documentation, because I'm not really familiar with the ci subcommand.

jgonggrijp added a commit that referenced this pull request Feb 11, 2022
@jgonggrijp
Copy link
Collaborator Author

Open Sauce doesn't seem to be really enabled. To be continued...

@ogonkov
Copy link
Contributor

ogonkov commented Feb 16, 2022

May be we can mark sause labs tests as non blocking for workflow and merge it in master, to fix units in PRs?

@jgonggrijp
Copy link
Collaborator Author

Yes that makes sense. Good idea. How do you do that?

@jgonggrijp
Copy link
Collaborator Author

jgonggrijp commented Feb 18, 2022

@ogonkov Turns out Open Sauce was enabled on the US cluster while my account is on the EU cluster. This has now been fixed. I decided to just disable IE 9, IE 10, Chrome 26 and Chrome 40 for the time being (#4253), as the selection of browsers that remains seems still better than nothing. If you agree with the current state of the code, we can merge this (I already rebased for cleanup).

@jgonggrijp jgonggrijp merged commit 91ab0de into master Feb 19, 2022
@jgonggrijp jgonggrijp deleted the patch-1 branch February 19, 2022 20:58
@jgonggrijp
Copy link
Collaborator Author

High five!

jgonggrijp added a commit that referenced this pull request Feb 25, 2022
The updated docco puts these annotated sources in a different place.
Rather than forcing them back in the old place, it seems easiest to
just go with the flow.
jgonggrijp added a commit that referenced this pull request Feb 26, 2022
The updated docco puts these annotated sources in a different place.
Rather than forcing them back in the old place, it seems easiest to
just go with the flow.
@jgonggrijp jgonggrijp mentioned this pull request Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants