Make NodeTest task use the TestRunnerTaskMixin

Review Request #3870 — Created May 11, 2016 and submitted

benjyw, stuhood

This lets NodeTest targets time out if they take too long.

Also addressing - adding the "--" to NodeTest and NodeRun to pass their passthru arguments correctly to NPM.

  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
  1. I have to run right now, here's some initial feedback. Thanks!

  2. If you can figure out how to get the TestFailedTaskError to only set failed_target to the target it is called with, that would definitely be better. I only did it like that in the first place because in python and junit the tests all get run together so there was no way to tell which test got timed out.

    1. Well, I can catch the exception there and re-raise a new one with the same message and correct failed_targets. Otherwise, I would have to change TestRunnerTaskMixin, because it is the thing that sets failed_targets to self._get_test_targets().
      The problem is that exception will still break out of the entire execute(). So my concern was if I set it to the particular target being executed, and the remaining targets in self._get_test_targets() are never run *or* named in failed_targets, that they'll just be "lost". Does that make sense?
      Also, looking at the code again, I think I might have to change the way it's working since you could technically name multiple NodeModule dependencies in a single node_test target (which will run the test script with the specified name against each NodeModule) and the Timeout will be applied against each dependency's run individually, rather than the target overall. (I might also just fix this by enforcing a single dependency per node_test for now, since that behavior with multiple dependencies is kind of weird and I doubt anybody is using it.)
    2. I have no objections to changing TestRunnerTaskMixin to make it better! It's a serious flaw in the current design that it aggregates the various targets and runs them all at once. I decided to punt on solving this problem immediately because in CI/Iron we never run aggregated targets, so you only get multiple targets being run when you have a test target that depends on another test target, which we warn against (and will eventually ban.)

    3. Alright, I created a somewhat ugly new method you can override for _spawn_and_wait to get the currently executing targets. It's difficult to do anything else, because _spawn_and_wait needs to pass all its arguments down to _spawn.
  3. why are you adding timeouts here?

    1. Because they had default 60 second timeout, and for running each of those targets individually (at least with ./pants clean-all) it was taking longer than that. (./pants test contrib/node:: actually ran without those timeouts, I guess because the sum of all the timeouts for all targets was enough when you include the other targets that run in much less than 60s).
    2. Aha right makes sense. I forgot about the default timeout in pants.ini.

  2. Actually using timers in tests is really quite fragile. The integration tests I wrote which don't mock out timers have been flaky and are now disabled, I still haven't had the time to fix them to not be fragile.

    (see for the fragile tests that are disabled due to flakiness)

    You should be able to mock or fake the timers, there are example tests that aren't fragile in test_pytest_run and test_junit_run

  3. I think this is actually because you haven't modified your node_tests target to include the timeout parameter. See for an example of the python_tests target.

  4. pants style requires full sentences and periods at the end of sentences.

  5. Please add a non-twitter reviewer. benjyw has been kind enough to spend a lot of time reviewing the test mixin code for me when I was developing it, so he may have the most context outside of Twitter.

  1. I'm no node expert, and the proof of this particular pudding is in the eating, so as long as this works...

  2. I'm no npm expert, so I'll take your word for it.

  3. Find another way to describe these args. It doesn't make sense to name a private method in the documentation of a public one.

    In reality these are args passed through to npm, no? That's what the reader cares about.

    Ditto in a couple of other places below.

    1. Changed it to say they go to the subprocess.Popen that runs node/npm (they go to that, not really node/npm).

    2. Well, but their purpose is to be passed through to npm, right? I don't think the reader needs to care that we use Popen as an implementation detail. Yes, the user could provide popen args that are not npm args, but if I understand correctly that's not what this is for, and we would frown on that. I think we want to document the intended usage, not emergent behavior that might change in the future.

    3. Ok, I just dropped those kwargs from execute_node and execute_npm completely: Now they're only exposed at the level of I'm not entirely sure why there were there in the first place. I was only using it in one test. I also cleaned up the way I was passing args as a kwarg.

      They really weren't being passed to npm. They were kwargs for subprocess.Popen, such as cwd and env. Like, I was using them in a test to have the command cwd itself. But it's probably better to not expose them.

    4. Gotcha. Yes, better not to expose them. Thanks for bearing with me on this change.

  1. Ship It!
  1. Thanks! looks great.

  2. i wonder if there is some cleaner way to do this setting of _currently_executing_test_targets in a context manager. I can't think of any way to do it right now.

    Not worth spending time on probably.

    1. Yeah, I recognize it's poor architecture. It's not catastrophic, though, and unfortunately I don't know a way around it that doesn't involve a major rewrite, which I definitely can't do right now.

    1. Is this alright?

    2. yeah just an RB fail on my part.

Review request changed

Status: Closed (submitted)

Change Summary:

Merged as e100ab534d01ddb706342e2c9594cf41c8bd89c8