fix scroll behaviour + [wip] cypress tests for scroll behaviour #8359
Conversation
|
Argh, of course my cypress tests locally but not in CI :) |
pieh
changed the title
|
Oh right duh. So Travis tests are going to fail, because I broke that with the CircleCI stuff. I'll fix that in a bit. It's weird, it looks like this PR somehow used the old CircleCI setup too 🙃 I'll take a closer look in a bit! This looks good though! |
Hmm, I'll merge master and see if that fixes circleCI test then --edit: |
|
@pieh they passed!! Except it's a false positive, haha. Basically the way it works is that the current commit's files are what trigger (or don't trigger) the e2e tests. I think/thought that was a good idea, but now we won't really know whether they're passing for real or not unless a change to packages/ or integration-tests/ comes through in a commit. I can also change that behavior, but the thought was that for each commit we should run only what's needed. i.e. one commit might change packages -> run e2e tests. Next commit might be a README.md change -> don't run e2e tests, etc. |
|
Oh, so it doesn't check file changes in PR - just file changes in latest commit? |
|
@pieh yup. But like I said, I can change that, open to other suggestions :) |
|
That makes sense as a default. This is an edge case. Is there a way to manually trigger tests to run for real? |
|
@KyleAMathews I think I can set up the e2e test suite as an approval workflow. Those tests wouldn't make the same check as to whether certain files changed. It'd be a bit of duplication in the job structure, but worth doing? |
|
Maybe yeah. But since this is such an edge case (not many people are going to be PRing from older code), @pieh could just push a commit with a minor change to trigger tests |
| "gatsby-plugin-manifest": "next", | ||
| "gatsby-plugin-offline": "next", | ||
| "gatsby-plugin-react-helmet": "next", | ||
| "gatsby": "latest", |
There was a problem hiding this comment.
👍 We should probably switch all versions in integration-tests to use this pattern now, too.
| } | ||
| } | ||
|
|
||
| getSnapshotBeforeUpdate(prevProps, prevState) { |
There was a problem hiding this comment.
Such a nit, but you're not actually using prevState are you?
There was a problem hiding this comment.
ah, yeah, will fix - it should also trigger actual tests, so that's good
Hmmm that's why CircleCi failed @DSchau |
|
@KyleAMathews I can SSH into the box, but 99% sure that's the |
|
Ok interesting — yeah, this sounds like a circleci wrinkle |
…byjs#8359) fixes gatsbyjs#7454 (again) but adding tests to catch regressions in future problem was temp hack for reach/router was disabled by recent changes because of order of calls changes - it's restored now
fixes #7454 (again) but adding tests to catch regressions in future
problem was temp hack for reach/router was disabled by recent changes because of order of calls changes - it's restored now