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

series, largest-series-product: Conflict when running all tests #344

Closed
arcuru opened this issue Jul 8, 2016 · 7 comments
Closed

series, largest-series-product: Conflict when running all tests #344

arcuru opened this issue Jul 8, 2016 · 7 comments

Comments

@arcuru
Copy link
Contributor

arcuru commented Jul 8, 2016

py.test gives you the option of running all the tests at once, which is part of the reason why it's used instead of the simpler unittest (#309).

When I actually try executing it I hit a problem. It appears that the "largest-series-product" problem reused the filename in the series problem (Both tests use from series import ..., so both tests expect a series.py). Pytest ignores one of them since you can't have two libraries with the same name.

(Pytest doesn't break if you use the example solution since it imports alphabetically and the example copies the needed function from the "series" problem, which might be what was intended when the problem was originally implemented, but that was taken out with #316.)

The fix for this would be trivial (change the import line in "largest-series-product), but I'm not sure if it's worthwhile breaking the ~380 existing solutions just to make the naming consistent.

Thoughts?

@arcuru arcuru changed the title series, largest-sum-multiples: Conflict when running all tests series, largest-series-product: Conflict when running all tests Jul 8, 2016
@kytrinyx
Copy link
Member

@Dog Would you mind taking a look at this?

@Dog
Copy link
Contributor

Dog commented Jul 25, 2016

I'll be able to take a closer look after Siggraph.

@Dog
Copy link
Contributor

Dog commented Aug 1, 2016

I believe it is a real issue, and that it is affecting more tests than just the two mentioned. For example, several exercises like rna-transcription and point-mutations expect a file dna.py. I did have pytest break with those two exercises, despite them passing in their respective folders.

Unfortunately I think the correct way to fix the problem may be to standardize the exercise files to follow the convention of <exercise_name>.py and <exercise_name>_test.py. This would also make it a lot easier to work with the repository as a whole. It was a headache when trying to copy all of the example.py files, and having to handle all of the special cases.

It wouldn't be a difficult change, but tedious. If we were to do that, we'd probably want to change all of them at once. The good news is that we aren't really changing functionality, just file names. This means a simple script could be written to migrate solutions.

For now I'd like to hold off on anyone picking this up for at least a week to get feedback and spend some more time considering the approach.

@kytrinyx
Copy link
Member

kytrinyx commented Aug 1, 2016

I think the correct way to fix the problem may be to standardize the exercise files to follow the convention of .py and _test.py

For the API to work (in order to not deliver the example solutions to people) we need the filename to be called something with example in it.

In some of the other tracks we have a makefile that copies the exercises to a temporary directory and renames the example files to the expected file name. I don't know if that would be appropriate here.

@Dog
Copy link
Contributor

Dog commented Aug 1, 2016

Its fine that the example solution is example.py. I'm suggesting we change the filename users create for their solution to be <exercise_name>.py. Sorry, I think its more clear what I am suggesting now that I have the markdown fixed above.

A makefile or python script could work for that. I was going to write a simple bash script if we were to standardize it. I guess what it comes down to then is if standardizing the names now buys us anything since it looks like we may have to break solutions, or if we should just break the ones required and manually handle the special cases with something like a script.

@kytrinyx
Copy link
Member

kytrinyx commented Aug 2, 2016

I would prefer to normalize and break a few things rather than keep going down a path that requires us to handle things manually or work around weird edge cases.

Dog added a commit that referenced this issue Aug 24, 2016
@Dog
Copy link
Contributor

Dog commented Aug 24, 2016

This issue should now be fixed. Please feel free to reopen if there is an issue.

@Dog Dog closed this as completed Aug 24, 2016
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

No branches or pull requests

3 participants