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

Disabling troublesome unit test. #301

Merged
merged 2 commits into from
Mar 6, 2019
Merged

Disabling troublesome unit test. #301

merged 2 commits into from
Mar 6, 2019

Conversation

jeffctown
Copy link
Collaborator

@jeffctown jeffctown commented Mar 6, 2019

Updating Readme with known issue.
Changelog.

Checklist

  • I've checked that all new and existing tests pass
  • I've updated the documentation if necessary
  • I've added an entry in the CHANGELOG to credit myself

Description

Disabling flaky tests and updating README with the known issue.

Motivation and Context

There has been an issue with redirects for several years that seems to be caused by an Apple bug. This issue has been investigated multiple times, and the path that we want to take forward is to disable the buggy test and update the README with the known issue.

Discussion on this topic has been mostly happening in #230 and #280 .

Updating Readme with known issue.
Changelog.
Copy link
Collaborator

@Liquidsoul Liquidsoul left a comment

Choose a reason for hiding this comment

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

👏

@jeffctown jeffctown merged commit 2322b10 into master Mar 6, 2019
@jeffctown jeffctown deleted the feature/BuggyTest branch March 6, 2019 17:34
@AliSoftware
Copy link
Owner

AliSoftware commented Mar 6, 2019

As commented in #298 maybe we should have kept that test enabled by default when run on Xcode and only disable it on CI (the same way I did with timing tests)?

@jeffctown
Copy link
Collaborator Author

@AliSoftware I think this is a great idea. I can make put up a pull request with that change soon, and it makes way more sense than just disabling it globally.

Would you prefer for me to wait for your approval on any of these pull requests before merging? You're bringing up totally valid concerns on them.

@AliSoftware
Copy link
Owner

In a dream world 🦄 I'd say if you could let me the day after you opened a PR to have a chance to review it before merging, that could be interesting (at least for the first PRs you take responsibility for) so that I can validate the direction…
But given how busy I am with induction into my new job I can't guarantee that I'll be as fast to react or review all your other PRs (tonight was just a light evening, not expecting many ^^) 😓

So given how you seem to have good motivation and you're helping unlocking a situation that I did let stale for too long (And thanks again for that 🙏), sure if there's a PR that can wait a few hours or the day then maybe you could wait so I might take a look before you merge… but in any case if you see I'm not reacting after one day (while @Liquidsoul approves the PR on his end), or a change is trivial, feel free to make things go forward without waiting for me — so that we don't continue having that stale situation on OHHTTPStubs that I kept postponing for too long for lack of my own availability. (We could always revert a PR if needs be in case of emergency after all)

@jeffctown
Copy link
Collaborator Author

@AliSoftware - Sounds reasonable to me. 👍 I can continue in this fashion.

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.

3 participants