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

Tests appear to be running in alphabetical order for Raindrops #2214

Open
kytrinyx opened this issue Aug 3, 2020 · 8 comments
Open

Tests appear to be running in alphabetical order for Raindrops #2214

kytrinyx opened this issue Aug 3, 2020 · 8 comments
Labels
discussion 💬 maintainer action required❕ A maintainer needs to take action on this. smolder🍲 Lower-priority and/or future idea

Comments

@kytrinyx
Copy link
Member

kytrinyx commented Aug 3, 2020

I tried out a couple of exercises on the Python track today, and noticed with the Raindrops exercise in particular that the tests were requiring me to implement the entire algorithm on the second failure.

I'm working in Python 3.8.5.

I run the tests with the following command:

pytest -x raindrops_test.py

With the unchanged stub file in place, I get the following failure:

    def test_2_to_the_power_3_does_not_make_a_raindrop_sound_as_3_is_the_exponent_not_the_base(
        self
    ):
>       self.assertEqual(convert(8), "8")
E       AssertionError: None != '8'

This is a reasonable "first" failure, since it's a single number that should be turned into a string.
If I make that pass with a hard-coded return "8", then this is the next failure I get:

    def test_the_sound_for_105_is_pling_plang_plong_as_it_has_factors_3_5_and_7(self):
>       self.assertEqual(convert(105), "PlingPlangPlong")
E       AssertionError: '8' != 'PlingPlangPlong'
E       - 8
E       + PlingPlangPlong

Looking at the tests, the order that they are defined in is quite nice, in terms of pushing the implementation forward step by step.

I tried adding the following declaration after the import unittest statement, to see if this would cause the tests to be run in the order that they are defined:

unittest.TestLoader.sortTestMethodsUsing = None

This did not change the order that the tests are run in.

If I sort the tests alphabetically by name, it appears to correspond to the order that I'm seeing when doing the exercise:

While under normal circumstances I would want tests to be run in random order, in Exercism exercises I think that it would make more sense to run the tests in the order that they are defined.

Is there a declaration we can make in order to accomplish this?
If not, then I would propose that we rename tests by adding a numeric prefix to the test name (e.g.

def test_01_the_sound_for_1_is_1(self):
@kytrinyx
Copy link
Member Author

kytrinyx commented Aug 3, 2020

I did a search, and it looks like there has been some discussion about this previously:

@iHiD
Copy link
Member

iHiD commented Aug 3, 2020

@yawpitch said in #1853 (comment):

We didn't do that for a good reason: we simply cannot rely on the canonical-data.json files having taken ordering into account in a manner that makes any sense to this sort of progressive TDD approach. Many tests have been added with no discernible concern for that sort of ordering, and I simply don't see us being able to apply such an ordering to that data, especially since that repo is still locked.

My quite take on this is that, while that's true and there are lots of places that the tests may not have been ordered correctly, there are also a significant amount of places where they have been deliberately ordered and where that order greatly improves the learning experience.

So there is value to respecting the order, although it will not always be beneficial, and I can't see the downside of not respecting the order.

If that is accurate, then the only negative of implementing some solution that respects the order is that (as outlined in #1853 and other places) there's not a particularly clean solution. However, I would argue that an ugly solution is better than not having ordering. So, pending someone willing to do the work, I think this is a change we should make.

@yawpitch
Copy link
Contributor

yawpitch commented Aug 3, 2020

Piping in to explain: the technical reason we never did this is because unittest.TestLoader.sortTestMethodsUsing = None does not imply line order sort, it merely removes entirely any current sort behavior (which is alpha by default)... so instead what you get is an undocumented and undefined order, though in practice in the current source code that’s the implicit sort order returned by calling dir(YourTestCaseSubclass) which, annoyingly, is alpha sort.

To reliably get line order you need to set the sortTestMethodsUsing class attribute to a function that looks up the line number in the source code of the original file via runtime introspection and compares based on that... such a function is non-trivial, involves importing at least the inspect module, and would need to be added to the template for every generated test file. It’s also fairly brittle if inheritance is used to cobble together unittest.TestCase instances, though I can’t think of an example of that in our code base.

All of that, and the fact that it just generally seems a bad idea to imply to students that they should expect a fully curated sort order in the real world of Python development, has weighed against this change.

Now, yes, another simpler way would be to add to the templates some test name generator logic that would introduce a sorting number (so each test method would be generated as “test_0001_some_aspect”, “test_0002_some_other_aspect”, etc), but we’d have to ensure zero padding for unknown numbers of tests, and we’ve already got a problem with overlong test names.

@iHiD
Copy link
Member

iHiD commented Aug 3, 2020

@yawpitch Thanks for explaining :)

In #1853 you mused about pytest saying:

Thus if we give a hard requirement on pytest as the only "blessed" test runner then wouldn't we be better off simply re-writing all the templates to generate pytest-style tests anyway, and we'd get the definition order for free in the process?

Is there a (good enough) reason not to use pytest as the "blessed" runner? I believe many other tracks do have a specific test runner that they advocate (the JS family as an example). It sounds like this would simplify the situation and maybe give us a cleaner solution? But I don't know enough about Python to understand the tradeoffs.

@cmccandless
Copy link
Contributor

cmccandless commented Aug 3, 2020

Is there a (good enough) reason not to use pytest as the "blessed" runner?

Reasons not to restrict to single test runner (I don't know if any of them are "good enough"):

  1. While we do endorse pytest, currently the test code only requires a supported Python installation; no other dependencies required.
    • However, if we're going to hold to that, perhaps the instructions shouldn't mention pytest at all (I am not in favor of moving in this direction)
  2. If a student is already familiar with a non-pytest runner, and just wants to practice with that, they are able to do so.

@AnAccountForReportingBugs
Copy link
Contributor

These exercises are supposed to teach the basics, so I think it makes more sense to have just unittest usage explained to avoid the issue of people having to figure out how to install pytest. I don’t really see the benefit of pytest over unittest here, other than slightly prettier output. If someone is already familiar with another runner, I would think they’d know how to use it already regardless.

@stale
Copy link

stale bot commented Sep 11, 2020

This issue has been automatically marked as abandoned because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the abandoned label Sep 11, 2020
@stale stale bot closed this as completed Sep 19, 2020
@BethanyG
Copy link
Member

I am going to re-open this issue. In light of V3 changes that now depend on PyTest and the test runner interface for V3 concept exercises having a task number and implementation tests (runner interface specs), it no longer makes sense to argue that we are "only" using unittest.

We now have dependancies on PyTest, and PyTest is at the heart of the runner that will be used on the V3 Website to run student code -- so we should instruct students to also install the same test runner and configuration on their own machines, so that they can use the same toolset prior to uploading.

PyTest is very popular (I'd almost call it a default at this point) and is used for most Python projects (outside of the Python language core dev team which uses unittest) -- nose has been in maintenance mode for a while, and no one has stepped forward to continue development. nose2 is also largely in a holding pattern. There is ptr - but it is pretty new. The vast majority of the instruction or tutorials a student might encounter would point them to PyTest, or PyTest + plugins. See this and this for some examples.

Not going to go into ordering atm, but pytest-order and pytest-ordering as plugins to PyTest look interesting as options.

@BethanyG BethanyG reopened this Jul 14, 2021
@BethanyG BethanyG added the maintainer action required❕ A maintainer needs to take action on this. label Sep 3, 2021
@BethanyG BethanyG added the smolder🍲 Lower-priority and/or future idea label May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 maintainer action required❕ A maintainer needs to take action on this. smolder🍲 Lower-priority and/or future idea
Projects
None yet
Development

No branches or pull requests

6 participants