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: work with deploy/netlify branch and existing #15

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Apr 2, 2019

Whew - this was kind of a doozy to get the unit tests passing.

Description

This PR introduces a fix for working on the newer (?) status check descriptions that Netlify exposes.

For example, if we check out the following repo we can see that the deploy preview status check is actually named deploy/netlify which will fail the regular expression /^netlify\/.*\/deploy-preview$/.

This fixes this issue, while remaining backwards compatible.

Additionally, I added a test and tweaked the existing tests to complete a little more quickly.

  • Removes nock and just mocks octokit/rest directly
  • Ensures that main.js is required new each test (jest.isolateModules)
  • Tweaks assertions slightly for consola.log statements

Let me know if you have any questions!

Related

Fixes #14

cc @amberleyromo

@codecov
Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #15 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #15   +/-   ##
=======================================
  Coverage   95.45%   95.45%           
=======================================
  Files           1        1           
  Lines          22       22           
  Branches        3        3           
=======================================
  Hits           21       21           
  Misses          1        1
Impacted Files Coverage Δ
main.js 95.45% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40fd1a6...d3731fc. Read the comment docs.

@@ -1,47 +1,95 @@
import nock from 'nock'
/* eslint import/first: "off" */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

jest.mock is hoisted--but it's a little cleaner to me by placing it at the top.

Happy to tweak this.

Copy link
Member

Choose a reason for hiding this comment

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

That's fine, thanks ☺️

@@ -7,7 +7,8 @@
"repository": "Developmint/wait-for-netlify-preview",
"scripts": {
"lint": "eslint index.js __test__",
"test": "yarn run lint && jest --detectOpenHandles",
"pretest": "yarn run lint",
"test": "jest --detectOpenHandles",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Longer term--if main.js is extracted out into a utility, this would be quite a bit easier to test and this option wouldn't be required.

Copy link
Member

Choose a reason for hiding this comment

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

If you want, you can do this in another PR 👍

@TheAlexLichter TheAlexLichter merged commit 7b54326 into Developmint:master Apr 3, 2019
@DSchau
Copy link
Contributor Author

DSchau commented Apr 3, 2019

@manniL thanks for the review and merge (and the utility!).

Could you let me know when this is published and in which version? Thank you!

@DSchau DSchau deleted the fix/deploy-netlify-branch branch April 3, 2019 13:11
@swyxio
Copy link

swyxio commented Apr 8, 2019

whoa, i just saw this. i still honestly have no idea what the problem is, but if this is a big deal for y’all, please escalate, make noise, and i’ll get someone smarter than me to look at this. if this fix is good, then im happy to leave it alone. just want to establish that we’re happy to help you when needed.

@amberleyromo
Copy link

amberleyromo commented Apr 8, 2019

@sw-yx nope, all good. I'm just trying to write up some stuff on automated accessibility testing. This fix works (I published a fork). I'm mostly just still curious about the reasons for the difference in context names.

#14 (comment)

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.

Regex fails to match successful deployment
4 participants