Converted wysiwyg jasmine file to jest test fixes #784 #785

NARUDESIGNS · 2022-01-11T16:39:04Z

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with grunt jasmine
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
PR body includes fixes #0000-style reference to original issue #
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

gitpod-io · 2022-01-11T16:39:07Z

jywarren · 2022-01-11T16:48:41Z

Hi, it looks like, since we recently merged in some new changes to the main branch, you need to rebase your work on top of the latest main. You can do that by following the process outlined here, but you may be able to skip step 2 if your main is still in sync with publiclab's main: https://publiclab.org/wiki/contributing-to-public-lab-software#Rewinding+the+master+branch

In general there are more tips and guides on rebasing here: https://publiclab.org/wiki/developers#Resources

Thanks!

jywarren · 2022-01-11T17:07:43Z

Looks like you're seeing real test failures now, not just an error before running. Let me know if you need help with these!

NARUDESIGNS · 2022-01-11T17:11:52Z

I ran the tests on my local machine and it all passed. How can I run it here please?
Or can you tell me the error it throws in the log?
@jywarren

jywarren · 2022-01-11T17:21:44Z

Sure - it's here: https://github.com/publiclab/PublicLab.Editor/runs/4778470519?check_suite_focus=true#step:7:49

What i do is click the Details link, then click the line in the log with the red error icon and it expands, then i search for "FAIL" to find individual failures. The log is tough to read so the search function really helps!

jywarren · 2022-01-11T17:23:13Z

One looks like a timeout, which is a little unexpected. Let's try to figure out why the others are true/false failures, and maybe the timeout will resolve itself? We can also just re-run it to see if it was something intermittent if it really is supposed to run smoothly. Finally, we could lengthen the timeout limit to see if it helps! But, my guess is it should be enough time, we're just waiting for local asynchronous code to run, which should be pretty fast.

NARUDESIGNS · 2022-01-11T17:35:47Z

This is a bit confusing. Line 49 refers to bold.test.js, all these tests passed before I started learning about them. I wonder how it is failing now when no changes was made to it.
@jywarren

jywarren · 2022-01-11T20:24:01Z

Ah, ok. So these test failures are totally unconnected to this PR. That's usually a signal we should a) restart them and b) look at other unrelated reasons the tests could be flaky, but in a separate issue. Would you mind opening one and linking specifically to this run, so we can check the logs? This URL shouldn't change even if we start the tests over:

https://github.com/publiclab/PublicLab.Editor/runs/4778470519

And you can link to this PR as an example of one where this happened.

jywarren · 2022-01-11T20:49:55Z

Yes, so confirming, this does seem to be intermittent, and unfortunately this time the CustomInsertTest failed:

FAIL test/ui-testing/CustomInsert.test.js (10.573 s)
  Custom Insert text
    ✕ Add Custom Insert text in rich text mode (10002 ms)

  ● Custom Insert text › Add Custom Insert text in rich text mode

    : Timeout - Async callback was not invoked within the 10000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 10000 ms timeout specified by jest.setTimeout.Error:

       7 | 
       8 | describe('Custom Insert text', () => {
    >  9 |   test('Add Custom Insert text in rich text mode', async () => {
         |   ^

My guess is we may need to restart it again, but also copy these errors into that new tracking issue and try to work the problem there. I'll also try running it in GitPod to see!

jywarren · 2022-01-11T20:59:28Z

test/ui-testing/wysiwyg.test.js

@@ -0,0 +1,53 @@
+const timeout = process.env.SLOWMO ? 60000 : 10000;


We can try increasing this to above 10 seconds for all tests...?

Sure we could do that. But may I ask please, why do you think we should increase the timeout? Did you experience any timeout recently?
@jywarren

Based on the message in the logs:

: Timeout - Async callback was not invoked within the 10000 ms timeout specified by jest.setTimeout.Timeout - Async callback was not invoked within the 10000 ms timeout specified by jest.setTimeout.Error:

I am guessing that for some reason it took longer. It's worth a try!

And that was the same as with one of the earlier bold test failures!

Oh ok, I get. I think I got that error sometimes and when I ran the test again it works fine. But it's ok, we could increase the timeout value to say 15000ms or 20000ms.
Is that much or enough, what do you think?

jywarren · 2022-01-11T23:33:24Z

I think if we increase it to 15 or 20 seconds and it doesn't help, then we at least know that our problem has to be solved another way, and it's not like we're just a second or two off. 5 or 10 seconds on each of... 6 tests? Isn't too bad, it's 30-60 seconds overall.

…

On Tue, Jan 11, 2022, 6:28 PM Paul Ibeabuchi C. ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/ui-testing/wysiwyg.test.js <#785 (comment)> : > @@ -0,0 +1,53 @@ +const timeout = process.env.SLOWMO ? 60000 : 10000; Oh ok, I get. I think I got that error sometimes and when I ran the test again it works fine. But it's ok, we could increase the timeout value to say 15000ms or 20000ms. Is that much or enough, what do you think? — Reply to this email directly, view it on GitHub <#785 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J2R64C52DJ5Z6QQAOLUVS4JZANCNFSM5LWY4NEA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

NARUDESIGNS · 2022-01-11T23:37:48Z

Alright. Lastly, can we try running that particular test again here?
Can you please teach me how to do it so I can try running again it to see if I'd always get that error? (just curious)

jywarren · 2022-01-11T23:49:18Z

For sure. Either you close and reopen the PR, or you can open the checks and you'll see a button:

Where it says Cancel workflow it'll later say "re-run" (once it's done). You can do it there too. You could cancel too but i'm always surprised by how long it takes to cancel.

jywarren · 2022-01-11T23:53:38Z

OK, i just pushed a commit lengthening all Jest test timeouts to 15 seconds instead of 10. Let's see... 🤞

jywarren · 2022-01-12T00:07:03Z

Well, 15 seconds doesn't seem to have worked :-(

This is weird because it just started happening, right? A variety of earlier PRs didn't see this issue?

I'm going to check if somehow there's an error happening inside the async test and not getting logged out. Maybe it's not a timeout but an error somehow...

jywarren · 2022-01-12T00:42:55Z

I think our next step is to research what's causing this timeout error. I started searching in https://github.com/facebook/jest/search?q=Timeout+-+Async+callback+was+not+invoked+within&type=issues - and bearing in mind https://jestjs.io/docs/asynchronous

I can try again later in the week!

…liclab/PublicLab.Editor into wysiwyg-jest-test-conversion Pulled changes from remote wysiwyg branch to local branch

NARUDESIGNS · 2022-01-13T16:25:10Z

Well, 15 seconds doesn't seem to have worked :-(

This is weird because it just started happening, right? A variety of earlier PRs didn't see this issue?

I'm going to check if somehow there's an error happening inside the async test and not getting logged out. Maybe it's not a timeout but an error somehow...

The PRs I created didn't have this issues and also my local tests passes as well. Where are you seeing these? I'm a little confused 🤕

jywarren · 2022-01-14T20:32:06Z

Just copying in a note here:

I think we need to read through some issues in the Jest repository, searching for keywords to find something similar to what we're seeing. We may also want to search a bit in Stack Overflow. Just trying to find someone who's seen something similar! We could also try to run things more verbosely so we get more log output as well... to try to determine when it's failing.

jywarren · 2022-01-18T16:51:29Z

Ideas

tests in parallel - adding a test somehow overloads the system? Or, this test is erroring, and the browser is then hung up when it's needed by the other tests?
make test runs 25 seconds long (not ideal but just in case)
Add a timeout to beforeAll - https://stackoverflow.com/questions/51048119/jest-async-callback-was-not-invoked-within-the-5000ms-timeout-specified-by-jes
start a completely new PR with this test to avoid caching... maybe superstitious :-)
catch errors within the async test: How to stop error UnhandledPromiseRejectionWarning Timeout Exceeded? puppeteer/puppeteer#3870 (comment)

…test passes

NARUDESIGNS · 2022-01-25T19:44:34Z

Hey @jywarren I think the solution is solid and we got all our answers. I just pasted the changes from #804, reduced the timeout from 60000 to 10000, added comment to explain that part and pushed here which was the original PR and it passes the test.
I think we solved it!

More info @ #804

jywarren · 2022-01-25T20:12:55Z

Super well done! This was a tough and mysterious one but you did great. 🎉

converted wysiwyg jasmine file to jest test

79a185c

Merge branch 'main' into wysiwyg-jest-test-conversion

3b075ea

Merge branch 'main' into wysiwyg-jest-test-conversion

d3a3e69

jywarren closed this Jan 11, 2022

jywarren reopened this Jan 11, 2022

jywarren reviewed Jan 11, 2022

View reviewed changes

lengthened all test timeouts to 15 seconds

2b31958

NARUDESIGNS added 2 commits January 13, 2022 14:40

cleaned up bracket placements

fb64b02

Merge branch 'wysiwyg-jest-test-conversion' of https://github.com/pub…

5371a78

…liclab/PublicLab.Editor into wysiwyg-jest-test-conversion Pulled changes from remote wysiwyg branch to local branch

NARUDESIGNS mentioned this pull request Jan 13, 2022

Add tooltip to Custom Insert button #782

Merged

5 tasks

NARUDESIGNS mentioned this pull request Jan 19, 2022

remade wysiwyg commit to push in a new PR #795

Closed

pasted data from the fix-wysiwyg-jest-conversion detection to see if …

f7ac5f7

…test passes

NARUDESIGNS and others added 2 commits January 25, 2022 20:46

deleted jasmine equivalent

40a0036

Merge branch 'main' into wysiwyg-jest-test-conversion

e9f701c

jywarren approved these changes Jan 25, 2022

View reviewed changes

jywarren merged commit 70feac2 into main Jan 25, 2022

This was referenced Jan 25, 2022

Fix wysiwyg jest test #804

Closed

[Planning] Refine the Public Lab Rich Editor (Outreachy) #760

Open

This was referenced Jan 26, 2022

Publish Button test in Jest #749

Closed

Add publish button test to Jest test suite #748

Closed

changed #759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converted wysiwyg jasmine file to jest test fixes #784 #785

Converted wysiwyg jasmine file to jest test fixes #784 #785

NARUDESIGNS commented Jan 11, 2022 •

edited

gitpod-io bot commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

NARUDESIGNS commented Jan 11, 2022 •

edited

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

NARUDESIGNS commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren Jan 11, 2022

NARUDESIGNS Jan 11, 2022

jywarren Jan 11, 2022

jywarren Jan 11, 2022

NARUDESIGNS Jan 11, 2022

jywarren commented Jan 11, 2022 via email

NARUDESIGNS commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 12, 2022

jywarren commented Jan 12, 2022

NARUDESIGNS commented Jan 13, 2022

jywarren commented Jan 14, 2022

jywarren commented Jan 18, 2022

NARUDESIGNS commented Jan 25, 2022 •

edited

jywarren commented Jan 25, 2022

		@@ -0,0 +1,53 @@
		const timeout = process.env.SLOWMO ? 60000 : 10000;

Converted wysiwyg jasmine file to jest test fixes #784 #785

Converted wysiwyg jasmine file to jest test fixes #784 #785

Conversation

NARUDESIGNS commented Jan 11, 2022 • edited

gitpod-io bot commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

NARUDESIGNS commented Jan 11, 2022 • edited

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

NARUDESIGNS commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren Jan 11, 2022

Choose a reason for hiding this comment

NARUDESIGNS Jan 11, 2022

Choose a reason for hiding this comment

jywarren Jan 11, 2022

Choose a reason for hiding this comment

jywarren Jan 11, 2022

Choose a reason for hiding this comment

NARUDESIGNS Jan 11, 2022

Choose a reason for hiding this comment

jywarren commented Jan 11, 2022 via email

NARUDESIGNS commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 11, 2022

jywarren commented Jan 12, 2022

jywarren commented Jan 12, 2022

NARUDESIGNS commented Jan 13, 2022

jywarren commented Jan 14, 2022

jywarren commented Jan 18, 2022

NARUDESIGNS commented Jan 25, 2022 • edited

jywarren commented Jan 25, 2022

NARUDESIGNS commented Jan 11, 2022 •

edited

NARUDESIGNS commented Jan 11, 2022 •

edited

NARUDESIGNS commented Jan 25, 2022 •

edited