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

Make files ending with _test show up, even if they are in the test/units folder #77

Merged
merged 2 commits into from Mar 2, 2012
Merged

Conversation

kenmazaika
Copy link
Contributor

Before the line of the test that was failing was being filtered out from the stacktrace if the test mode was unit. This was causing a stacktrace that was this:

/Users/testman/.rvm/rubies/ruby-1.9.3-p0/lib/ruby/1.9.1/test/unit/assertions.rb:185:in `assert_equal'
/Users/testman/source/campaign_manager/test/unit/omg_test.rb:145:in `block in <class:OmgTest>
/Users/testman/source/campaign_manager/app/models/omg.rb:145:in `in double_rainbows`

to be filtered to this:

/Users/testman/source/campaign_manager/app/models/omg.rb:145:in `in double_rainbows`

and not give what line of the test file the assertion or exception failed at. This was caused by the filter_backtrace method removing all the elements of the backtrace that are in the test/units directory (and not only the ones that were specific for rails infrastructure test directory like assertion.rb).

This change does the following:

  • Adjust the regex, so if the user follows the rails convention of naming their test file ending with _test.rb, that part of the backtrace will not be filtered out.
  • Add a unit test for this case. The current tests seemed to all shell out to call the executable directly. Since the test I wanted to add was at the level of the Reporter class, I made a new test file for this, because it didn't seem to belong with the other tests.

Let me know if there are any changes that I need to make before getting this merged into master.

@trans
Copy link
Contributor

trans commented Feb 22, 2012

I'll accept, but you make it clear to me this really needs a more robust solution. If you have any ideas, please convey.

@kenmazaika
Copy link
Contributor Author

These are the ideas I have:

  • Smarter RegEx - we could build a smarter regex based on more information. If we can figure out the path of the unit test libraries, we can filter out any that have that path
  • Using the Current Project Root - if we can figure out what the current project root is, we can filter all of the entries that come before the first project root in the array, these will be the test framework. Any non current project root at the end of the list we would want to keep, because these are third party gems that threw exceptions, and we would want to surface these.
  • Copying how a different test framework does it - people who worked on other frameworks have probably thought about this problem. We could investigate how, rspec, minitest, testunit, etc, work and pick the solution from there that seems best.
  • Disabling filtering - while it's nice to have the line number of the test file that failed at the top of the stack trace, I think having the line number of the test that failed always in the stack trace is more important than filtering out extra stuff. We could set the default to disable filtering, and make some sort of configuration option that advanced users could use.
  • Some hybrid solution - starting with one solution, then falling back to a different one. For example we may be able to get Rails.root if it's a rails project, but not get the project root if it is not. In this case we could go with the current root project primarily, and then fallback to a different solution if it is not a rails project.

I think figuring out the current project directory, and filtering based off of that is probably the strongest way, if we can get that to work (seems simpler than specific code for each of the test drivers).

I'm happy to help implement the solution, if you know what we should go for.

@trans
Copy link
Contributor

trans commented Feb 28, 2012

I like your conclusion. Feel free to work on it, or I'll have a go at it when I get back from 3-day trip.

But the default should be filtered and a config option to not filter (I've had complaints about not filtering before). If memory serves, $DEBUG will already do this. Is that good enough, or do you think it should have specialized config?

@kenmazaika
Copy link
Contributor Author

I think using the $DEBUG option as the only way to turn on or off filtering makes sense. Adding additional config options will only add complexity.

What do you think about merging in this pull request, so it works by out of the box for people using rails conventions, and then also documenting the $RUBY_IGNORE_CALLERS variable in the README. That way if the same problem hits someone else, they will know where to look, and hopefully can help make smarter regexes over time as different people update it to support their edge cases.

I think it's important to not filter out the test line for people using common ruby conventions though.

@trans
Copy link
Contributor

trans commented Feb 29, 2012

Okay, we can do that. But could you make is also support files starting with 'test_' b/c that is what I usually do. Then I will merge, and we can go from there.

@kenmazaika
Copy link
Contributor Author

Sure, that makes sense. I'll tweak the regex, push up the changes, and make another comment on the pull request so you'll get notified.

I should get around to it in the next day or two.

@kenmazaika
Copy link
Contributor Author

I just updated the pull request:

  • Updated RegEx to not filter out files starting with 'test_'
  • Add test case that backtrace filtering does not remove unit test files from the backtrace if they meet this condition

trans added a commit that referenced this pull request Mar 2, 2012
Make test_* and *_test files show up, even when in the test/units folder
@trans trans merged commit 024f34c into turn-project:master Mar 2, 2012
@trans
Copy link
Contributor

trans commented Mar 2, 2012

Pulled, thanks!

Btw, don't quite understand the trailing .*. Instead of

/\/test\/unit(?!(\/.*\_test.rb)|.*\/test_.*).*\.rb.*/

wouldn't

/\/test\/unit(?!(\/.*\_test.rb)|.*\/test_.*).*\.rb/

be enough?

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