Replace trace-related fixtures with factories. #1347
Conversation
The mocking and stubbing in the controller tests is unfortunate, but the models interact directly with the filesystem using the trace id so that's hard to control any other way.
This is a much cleaner approach than before.
|
I've reworked the stubbing slightly today, to involve a lot less gnarliness. |
There was a problem hiding this comment.
I'm really confused by all the stubbing as there doesn't seem to be any obvious need to stub things that just return strings like large_picture_name?
I got the impression it was related to the mime ype stuff but as the files exist I don't see any reason why that wouldn't work in the test environment?
| # And finally we should be able to do it with the owner of the trace | ||
| get :view, { :display_name => users(:public_user).display_name, :id => 5 }, { :user => users(:public_user).id } | ||
| # And finally we should not be able to view a deleted trace | ||
| deleted_trace_file = create(:trace, :deleted) |
There was a problem hiding this comment.
This should be created at the start of the method or the previous tests aren't really testing what they are supposed to test.
There was a problem hiding this comment.
To be honest I think the testing a deleted trace should be a different test entirely from testing the non-existent ones, but I've moved the creations to the top of the methods anyway.
| get :data, { :display_name => users(:normal_user).display_name, :id => gpx_files(:public_trace_file).id }, { :user => users(:normal_user).id } | ||
| check_trace_data gpx_files(:public_trace_file) | ||
| public_trace_file = create(:trace, :visibility => "public", :user => users(:normal_user)) | ||
| Trace.stub_any_instance :trace_name, "#{GPX_TRACE_DIR}/1.gpx" do |
There was a problem hiding this comment.
Don't we need to replace 1.gpx here with whatever trace ID the factory allocated? That is what check_trace_data expects?
| @@ -353,38 +367,45 @@ def test_data_not_found | |||
| assert_response :not_found | |||
|
|
|||
| # Now with a trace that has been deleted | |||
| get :data, { :display_name => users(:public_user).display_name, :id => 5 }, { :user => users(:public_user).id } | |||
| deleted_trace_file = create(:trace, :deleted) | |||
There was a problem hiding this comment.
As before this needs to go at the start of the method.
| trace.destroy | ||
| assert_equal "trackable", users(:public_user).preferences.where(:k => "gps.trace.visibility").first.v | ||
| public_trace_file = create(:trace, :visibility => "public") | ||
| public_trace_file.stub :trace_name, "#{GPX_TRACE_DIR}/1.gpx" do |
There was a problem hiding this comment.
This seems different to the way other ones are stubbed?
| trace.destroy | ||
| assert_equal "private", users(:second_public_user).preferences.where(:k => "gps.trace.visibility").first.v | ||
| public_trace_file = create(:trace, :visibility => "public", :user => users(:normal_user)) | ||
| public_trace_file.stub :trace_name, "#{GPX_TRACE_DIR}/1.gpx" do |
There was a problem hiding this comment.
Another one where the stubbing is different.
|
So the basis of the stubbing is that we need to test against specific gpx files that are found in the The Trace objects access the filesystem directly, based on their own object id's. We can't control the ids via factorygirl, and they aren't using any other attributes that we could control when creating the objects. So we need to find something to stub, so for a particular test we can say "use this specific gpx file". app/models/trace.rb shells out repeatedly while reading xml files. We could stub out all of the system and backtick calls to make sure the appropriate file is used, or since each system/backtick uses Similarly for other things like testing icon_picture and large_picture methods - we could stub out the File.new calls, or stub the method that generates the paths. I chose the latter. Finally, there is an additional twist for some of the calls, where I've used stub_any_instance. This is where the controller method being tested uses A completely different approach would be to create the test instances, and then copy around the necessary gpx files within the test directory and rename the files to give them the ids assigned by factorygirl. But that seems even worse to me. I'd consider using FakeFS or similar to mock out the filesystem calls, but I doubt that would work with the stuff we're doing backticking out to /usr/bin/file. I'm happy to refactor if there's a preferred way to do things! |
|
I don't know why the travis check failed with test errors. I've ran the suite again locally and can't reproduce the failures. Do they fail for anyone else? |
|
Might mean it depends on the order the tests are run in - try running the one that failed in travis on it's own (add TEST=path/to/file.rb to the command line). |
|
I ran the tests again, and with the seed that failed on travis, but couldn't reproduce the bug. The three failing tests were all regarding the same on-disk file. Running travis again just now shows the tests are passing. At this point I blame cosmic rays. |
|
Oh, I think I've figured it out. In some places in the tests, we do things like If the created trace has an id <= 10 , then one of the fixture files from will be deleted. Gah. So it's not only execution-order-dependent, it only happens with the primary key sequence is reset, which is why I was struggling to reproduce it locally. I'll need to think about this, suggestions welcome. |
This prevents them from being deleted by mistake, if trace.delete is called on a factory-generated trace with a coincidental id.
required number of results. Previously, the tests would pass regardless of whether anything was in the database.
|
I've fixed the bug by using letters instead of numbers for the gpx fixtures. This way they won't be accidentally deleted during the test runs. I spotted an error in the refactoring of the trace_controller test, since the check_trace_list method didn't have an expected number of results. This meant that it failed to assert on empty lists. The timestamp thing in that commit I spent ages faffing around with. If I remove the 4.seconds.ago stuff, then the test fails for me consistently and I'm not 100% sure what's going on. But making the timestamps explicitly different means the |
I think this is an improvement compared to running db queries to fetch the expected results.
|
So I've had another look through this and it's mostly fine but I'm still not convinced the stubbing approach is right, and I thought I had come up with a better plan then I saw what @gravitystorm wrote above:
Which is pretty much exactly the plan I had come up with... To have a set of fixtures (yes I know...) which are the various files we want to be able to use and a separate directory which is the active spool being used by rails and when we need to we tell factory girl which data to use and it copies it over, otherwise it just creates default empty files or something. It seems to me that is a more accurate test in that it doesn't wind up with us testing the stubs rather than the real code, and it more accurately models things - a trace that exists in the database should have corresponding files on disk - that is basically an expected invariant of the code - so when the factory creates a trace object it should also create the corresponding files. So I'm interested in why you think it's a worse solution? |
Mainly just the slowdown from having to write files to disk (when they already exist elsewhere on the disk anyway), which is a big slowdown compared to stubbing things, especially when the models only need to read from disk anyway. Then if we're writing stuff to disk during a test, we need to make sure we clean everything up again. With the stubbing, when the tests finish (or are interrupted) there's nothing permanent. So that's why I prefer stubbing to shifting things around on disk. But if you'd prefer to do the disk approach I can do that. |
@tomhughes I think we should either: a) merge this as-is, or |
|
I've merged this as is and also opened #1426 to demonstrate my alternate suggestion of how to handle the trace file issue. |
The mocking and stubbing in the controller tests is
unfortunate, but the models interact directly with the filesystem
using the trace id so that's hard to control any other way.