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

Remove unused AV/test/fixtures/happy_path #17877

Merged
merged 1 commit into from
Dec 2, 2014

Conversation

claudiob
Copy link
Member

@claudiob claudiob commented Dec 2, 2014

The test/fixtures/happy_path/render_action/hello_world.erb file was introduced in 8ab37c7 for the TestRenderAction test.

That test was subsequently removed in 34f058e, so the fixture is not used anymore.

Once Travis CI is happy with this PR, you can be sure the fixture can be removed.

The `test/fixtures/happy_path/render_action/hello_world.erb` file was
introduced in 8ab37c7 for the `TestRenderAction` test.

That test was subsequently removed in 34f058e, so the fixture is not
used anymore.

If Travis CI is happy, then you can be sure the fixture can be removed.
@senny
Copy link
Member

senny commented Dec 2, 2014

@claudiob would you mind removing all the surplus fixtures in one go? This will save running time for Travis and keep our git history easy to follow. (#17878)

@claudiob
Copy link
Member Author

claudiob commented Dec 2, 2014

@senny I'm sorry about that! 😓

Whenever I find a file that is possibly bound for deletion, I go back in the git history to find references to the original commits and add all the details in the message so reviewers can double-check that my commit is correct.

This approach results in more but smaller (atomic) commits, which are shorter to read, double-check and approve/reject.

This is the kind of PR that I like to receive on my own repos, so I thought you'd appreciate the same style. If you are more oriented towards an aggregated commit, with a longer message dealing with multiple files at once, then I'll go that direction from now on! 😉

@senny
Copy link
Member

senny commented Dec 2, 2014

@claudiob we generally try to bundle related removals together. Since this looked like the fourth pull request, which removes unused fixtures I assumed you are specifically looking into which fixtures are no longer used. So my thought was, we might as well remove them in one go.

senny added a commit that referenced this pull request Dec 2, 2014
Remove unused AV/test/fixtures/happy_path
@senny senny merged commit 3b6f1db into rails:master Dec 2, 2014
@claudiob claudiob deleted the remove-happy-path-fixture branch December 2, 2014 16:17
@claudiob
Copy link
Member Author

claudiob commented Dec 2, 2014

Understood! I'll do that next time! Thanks 🙋

@claudiob
Copy link
Member Author

claudiob commented Dec 2, 2014

@senny On a separate note… have you ever thought of adding a --fail-fast option to the tests on Travis CI?

In other words, as soon as one test fails, the execution on Travis is stopped, and the next PR starts getting tested.

I use this trick on my own repos, so failing PRs do not take a long time to fail. Sure, you won't see all the errors that made a PR fail, only the first one, but I think the real goal of Travis is not to show you all errors (which you can do locally), but just to be a semaphore.

Tell me if you have any opinion about this… if you'd like I can port this discussion to the Google Group!

@rafaelfranca
Copy link
Member

I prefer not, or people will fix the actionpack suite but forgot about the
railties tests for example.
On Dec 2, 2014 4:04 PM, "Claudio B." notifications@github.com wrote:

@senny https://github.com/senny On a separate note… have you ever
thought of adding a --fail-fast option to the tests on Travis CI?

In other words, as soon as one test fails, the execution on Travis is
stopped, and the next PR starts getting tested.

I use this trick on my own repos, so failing PRs do not take a long
time to fail. Sure, you won't see all the errors that made a PR fail,
only the first one, but I think the real goal of Travis is not to show you
all errors (which you can do locally), but just to be a semaphore.

Tell me if you have any opinion about this… if you'd like I can port this
discussion to the Google Group!


Reply to this email directly or view it on GitHub
#17877 (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.

None yet

3 participants