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

[MRG] DOC ask for non-regression test for bug-fix #8850

Merged
merged 9 commits into from
May 23, 2017

Conversation

massich
Copy link
Contributor

@massich massich commented May 9, 2017

Reference Issue

Fixes #8679

What does this implement/fix? Explain your changes.

This PR takes over the PR #8817 'cos it seems dead

Any other comments?

@massich massich changed the title DOC ask for non-regression test for bug-fix Issue #8679 [MRG] DOC ask for non-regression test for bug-fix May 9, 2017
@glemaitre
Copy link
Member

LGTM

@jnothman
Copy link
Member

jnothman commented May 9, 2017 via email

@naoyak
Copy link
Contributor

naoyak commented May 10, 2017

Travis failure is unrelated and should be fixed by re-triggering the build: https://github.com/kennethreitz/requests/issues/4006

@massich
Copy link
Contributor Author

massich commented May 10, 2017

@jnothman I see. I'll do next time. My apologies to @puneetmathurDS.

@lesteve
Copy link
Member

lesteve commented May 10, 2017

2 comments:

  • I would check CONTRIBUTING.md as well to see whether the same information can fit there as well
  • Maybe I would very quickly explain what a regression test is (basically fails on master, pass on your feature branch). People who need to read these instructions may not be familiar with what a regression test is.

@massich
Copy link
Contributor Author

massich commented May 10, 2017

@lesteve CONTRIBUTING.md has no section regarding converage. Only a bullet list telling you how to check coverage. However, both CONTRIBUTING.md and contributing.rst have a section regarding BUGFIX, Shall we explain the non-regression test there?

BTW, is there anyway to link RST and MD files so that only one needs to be edited?

@lesteve
Copy link
Member

lesteve commented May 10, 2017

@lesteve CONTRIBUTING.md has no section regarding converage. Only a bullet list telling you how to check coverage. However, both CONTRIBUTING.md and contributing.rst have a section regarding BUGFIX, Shall we explain the non-regression test there?

Use your best judgement.

BTW, is there anyway to link RST and MD files so that only one needs to be edited?

Nope, this is a bit unfortunate but there you go.

Side-comment: I am not 100% sure about this but because we use squash and merge it is very likely that the commits in master will appear under the name of the first committer. You did not really use anything in the original PR so starting from scratch was probably the best way forward.

@massich
Copy link
Contributor Author

massich commented May 10, 2017

Actually both have a Filing Bug section not a BugFix section. If you file a bug you don't have to supply a non-regression test. So it should not go here.

@massich
Copy link
Contributor Author

massich commented May 10, 2017

I've moved the explanation to somewhere that was making more or less sense and that it was common to both files. Maybe needs some rewriting.

@massich
Copy link
Contributor Author

massich commented May 11, 2017

I addressed the comments. (see artifact )

ping @lesteve

@lesteve
Copy link
Member

lesteve commented May 11, 2017

The best place I could find where to add this piece of information was in the same bullet point as "Documentation and high-coverage tests are necessary for enhancements to be accepted."

@massich
Copy link
Contributor Author

massich commented May 15, 2017

Moved right after @lesteve suggested sentence. (see artifact)

CONTRIBUTING.md Outdated
to be accepted.
- Documentation and high-coverage tests are necessary for enhancements to be
accepted. Bug-fixes or new features should be provided with non-regression
tests. Tests that verify the correct behavior of the fix or feature. In this
Copy link
Member

Choose a reason for hiding this comment

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

"Tests that verify" -> These tests verify

@@ -222,6 +222,11 @@ rules before submitting a pull request:

* Documentation and high-coverage tests are necessary for enhancements
to be accepted.
Bug-fixes or new features should be provided with non-regression tests.
Tests that verify the correct behavior of the fix or feature. In this
Copy link
Member

Choose a reason for hiding this comment

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

"Tests that verify" -> These tests verify

@amueller
Copy link
Member

I think in the world of voluntary contributions, presuming something dad
after a week without asking is a bit ... presumptuous

By that measure I'm long gone to dust ^^

@amueller
Copy link
Member

LGTM, maybe link to wikipedia for regression test?

@massich
Copy link
Contributor Author

massich commented May 22, 2017

ping @amueller @lesteve

CONTRIBUTING.md Outdated
to be accepted.
- Documentation and high-coverage tests are necessary for enhancements to be
accepted. Bug-fixes or new features should be provided with non-regression
tests. Tests that verify the correct behavior of the fix or feature. In this
Copy link
Member

Choose a reason for hiding this comment

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

I would be even more explicit for bug-fix to say that the test needs to be failing on master and passing in the PR branch.

@lesteve
Copy link
Member

lesteve commented May 23, 2017

I think this is good enough, let's not spend more time nitpicking on this one. Merging, thanks a lot!

@lesteve lesteve merged commit 1b5bff3 into scikit-learn:master May 23, 2017
@massich massich deleted the is8679 branch June 1, 2017 09:44
Sundrique pushed a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
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.

Add short explanation of regression test do contributing docs.
7 participants