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

Avoid running test for #1456 on Safari #1481

Merged
merged 1 commit into from
Jul 15, 2017

Conversation

fatso83
Copy link
Contributor

@fatso83 fatso83 commented Jul 4, 2017

Purpose

This avoids breaking the build by avoiding running the breaking test on Safari. See background in #1462.

Background (Problem in detail) - optional

Safari throws an error when trying to configure properties on window that are not configurable. AFAIK this is not a problem Sinon can solve: if the prop cannot be stubbed, then we shouldn't suppress the error or avoid trying to stub it.

How to verify - mandatory

  1. Check out this branch
  2. npm install
  3. Configure Sauce Labs:
export SAUCE_USERNAME=sinon-sauce-name
export SAUCE_ACCESS_KEY=abcd3-12345-...
  1. npm run test-cloud

BTW, the code is not so nice ATM. I was just so annoyed by the errors from Travis, so just needed to put a fix out there ... Not totally sure where to put the runtime detection of how this breaks and I am not so sure about how I skip the test. Maybe a simple return would suffice?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.998% when pulling 286b0a2 on fatso83:fix-safari-breaking into 1108117 on sinonjs:master.

@coveralls
Copy link

coveralls commented Jul 4, 2017

Coverage Status

Coverage remained the same at 94.998% when pulling 54f67c1 on fatso83:fix-safari-breaking into 1108117 on sinonjs:master.

describe("#1456", function () {
if (throwsOnUnconfigurableProperty) { return; }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to do this all inline instead of setting a single-use variable?

Also if you do the check in the beforeEach, you can call this.skip(); to skip all the related tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, it was a quick and dirty thing, and I wasn't aware of the this.skip thing.

Copy link
Member

Choose a reason for hiding this comment

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

@fatso83 will you make a slow and clean version, or shall we merge this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, sorry, I totally forgot. Was offline and on holiday for a week.

@fatso83 fatso83 force-pushed the fix-safari-breaking branch 2 times, most recently from 96023d8 to 3642905 Compare July 13, 2017 10:09
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.002% when pulling 3642905 on fatso83:fix-safari-breaking into 717ebf7 on sinonjs:master.

@fatso83
Copy link
Contributor Author

fatso83 commented Jul 13, 2017

I think the issues have been addressed, @fearphage. I started moving the test for error throwing to beforeEach, but it was a lot of detail that needed commenting, so opted to extracting it to a named function instead to document the intent.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.002% when pulling bb2d585 on fatso83:fix-safari-breaking into 717ebf7 on sinonjs:master.

@coveralls
Copy link

coveralls commented Jul 13, 2017

Coverage Status

Coverage remained the same at 95.002% when pulling 24d15f2 on fatso83:fix-safari-breaking into 717ebf7 on sinonjs:master.

@mroderick
Copy link
Member

👍

@mroderick mroderick merged commit 259a330 into sinonjs:master Jul 15, 2017
@fatso83 fatso83 deleted the fix-safari-breaking branch August 1, 2017 17:25
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.

4 participants