Skip to content
This repository was archived by the owner on Oct 11, 2022. It is now read-only.

Conversation

@mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Jun 15, 2018

Status

  • WIP
  • Ready for review
  • Needs testing

Related issues (delete if you don't know of any)
Replaces #2843, based on #3243, also based on #3382 (so until that's merged you'll also see those changes here)

This basically means we're running integration tests in the production setup, which is a big deal. ™️ We should (in a follow-up PR) also be able to configure Cypress to fail tests on SSR checksum mismatches (in case that doesn't happen yet) so we catch those as soon as they happen instead of months after the fact by chance!

  • Make Cypress run against hyperion
    • in CI
  • Make all the tests pass
    • 3 failing in community_settings_billing_spec.js
    • 1 failing in home_spec.js
    • 1 failing in inbox_spec.js
    • 2 failing in navbar_spec.js
    • 1 failing in thread_spec.js
  • Update docs

@spectrum-bot
Copy link

spectrum-bot bot commented Jun 15, 2018

Warnings
⚠️

These modified files do not have Flow enabled:

  • config-overrides.js
  • shared/middlewares/thread-param.js

Generated by 🚫 dangerJS

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 15, 2018

They are runninnnng!!!

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 15, 2018

@brian-mann CircleCI errors out because "Too long with no output (exceeded 10m0s)", but there was Cypress output to the console the whole time 🤔 Any clues why that happens?

@brian-mann
Copy link

It means something happened when trying to spawn the browser to run thread/delete_spec.js...

Doesn't make sense considering all of the other specs spawned correctly. Rerun CI a few times to see if it continually happens.

You can also try setting the DEBUG logs which will output a massive stream of data but will give details as to where Cypress was when this happened.

DEBUG=cypress:* cypress run

Another option is to try Chrome although that won't record a video. We automatically detect things like Electron crashes, and we automatically handle when the browser doesn't connect (we will correctly time out). So what I'm thinking here is that the browser actually did launch, but that something failed to load related to Cypress, since it doesn't appear that your tests ever began.

Try rerunning so we can see if it's just that spec file.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 16, 2018

Now they ran through, must've been a fluke! Thanks for the response 🆗

@brian-mann
Copy link

Other users are reporting the same problem such as this: cypress-io/cypress#1934

There is something we need to look at and fix.

mxstbr added 2 commits June 18, 2018 13:37
@brianlovin I changed your test-payments stuff to work with :3006
instead of :3001 so that it works when running integration tests against
hyperion!
@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 18, 2018

All of the remaining tests fail due to the same error: "SecurityError: Blocked a frame with origin 'http://localhost:3006' from accessing a cross-origin frame." Unfortunately, the stack trace is totally useless in debugging why this happens:

screen shot 2018-06-18 at 13 52 16

Interestingly, this happens to all the tests that load the inbox view. @brianlovin do you have a hunch what this could be? What is the inbox view loading that could cause this, are we redirecting somewhere?

@brian-mann have you seen this before/could that error be made to be a bit more helpful? Right now, I'm stuck here, not knowing why this is happening or what I have to do to fix it.

mxstbr added 8 commits June 23, 2018 13:00
This was necessary when we didn't use `react-loadable` for SSR, but now
that we do this is entirely unused so we can just get rid of it!
@brianlovin this _should_ fix the ?t param even when loaded with SSR,
right? The issue previously was that we'd mount with an active thread,
but the dashboard would still switch to the first one that's loaded once
the thread feed loads.
We unset the mounted thread ID before any threads were loaded, so when they finally did load in mountedThreadId was null and the app thought it had to select the first thread. This fixes it by only doing the "unset mounted thread id" logic after threads have already loaded! 🎉
Also refactor render code a bit to be nicer
This should fix the issues with the SecurityError's from #3334
@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 25, 2018

I thought disabling chrome web security would fix it, but apparently it didn't... 🤔

EDIT: Nevermind, now only the navbar specs are failing!

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 25, 2018

I think the remaining tests are failing because the ServiceWorker is caching HTML pages instead of going to the server-side rendering server or serving the app shell.

@NekR we want to do the standard app shell pattern where the first visit to a page returns the server-side rendered HTML, then the SW caches an app shell (/index.html) and then all future visits return that app shell, which loads fresh data from the API. How can we tell the offline-plugin to not cache the server-side rendered HTML, and only the app shell? What are we doing wrong?

new OfflinePlugin({
appShell: '/index.html',
caches: process.env.NODE_ENV === 'development' ? {} : 'all',
externals,
autoUpdate: true,
ServiceWorker: {
entry: './public/push-sw.js',
events: true,
prefetchRequest: {
mode: 'cors',
credentials: 'include',
},
},
AppCache: false,
})

mxstbr added 3 commits June 25, 2018 16:27
Omgggg @brianlovin @uberbryn I think I finally fixed that gosh darn
ServiceWorker error we've been constantly getting AND this might also
fix those cached notifications pages we've been
seeing!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 25, 2018

@NekR that last commit fixes it: a4f345d

The issue was that our app shell lives at /index.html, which the default rewrites method turns into /—but the app shell doesn't get returned for /, only for index.html! I think the appShell option shouldn't be passed through rewrites, that was very very annoying to debug? Or at least document the default rewrites method and make it clear that appShell is being run through it.

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 25, 2018

Now I'm getting "Timed out waiting for the browser to connect." for all tests 🤔

mxstbr added a commit that referenced this pull request Jun 25, 2018
This is cherry-picked from #3334 since that's not quite done yet,
but this should go live asap.
@mxstbr mxstbr mentioned this pull request Jun 25, 2018
3 tasks
@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 25, 2018

Since it seems like it'll take a bit longer to figure out Cypress 3.0 I've moved the ServiceWorker fix to #3384, which we should ship asap.

@NekR
Copy link

NekR commented Jun 25, 2018 via email

@brianlovin brianlovin closed this Aug 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants