Move `files` argument to `api.run()` #536

sotojuan · 2016-02-09T16:26:48Z

This fixes #534. Of course, it requires everyone to be on board with the idea of moving files to api.run() instead of the constructor. In case everyone agrees, then this should help. Let me know if I missed anything.

mention-bot · 2016-02-09T16:26:50Z

By analyzing the blame information on this pull request, we identified @novemberborn, @ariporad and @jokeyrhyme to be potential reviewers

novemberborn · 2016-02-09T16:43:40Z

Sadly this breaks --watch. To be fair there is no actual integration test from cli.js with --watch so you wouldn't have noticed. The test for lib/watcher stubs all its dependencies. I've opened #537.

files and excludePatterns were assigned so lib/watcher.js could reference them. You'll have to pass them to start() now so the watcher can run the initial tests or specific tests. This probably means defining the defaults in cli.js or perhaps as an export from api.js for programatic convenience. They no longer need to be stored on the API instance. (Looks like it'd be valid to retain the excludePatterns definition in the API constructor though).

explicitTitles is there so the watcher can force a test title prefix even when running a single test. Your change makes it so test titles are always prefixed. If we're willing to go with that we can clean up the code. Otherwise run() should take a second forcePrefixTestTitles argument the watcher can use.

sotojuan · 2016-02-09T18:21:54Z

Exporting files and excludePatterns could work but it'd require a big change in a lot of files (I think), so we'd have to get permission from the powers that be (@sindresorhus @jamestalmage).

Some more exploration/updates:

This makes the watcher work:

watcher.js

exports.start = function (logger, api, files, excludePatterns, sources, stdin) {
  var isTest = makeTestMatcher(files, excludePatterns);
  var patterns = getChokidarPatterns(sources, files);
  ...
}

cli.js

var files = cli.input.length ? cli.input : arrify(conf.files);

if (files.length === 0) {
  files = [
    'test.js',
    'test-*.js',
    'test'
  ];
}

var excludePatterns = [
  '!**/node_modules/**',
  '!**/fixtures/**',
   '!**/helpers/**'
];

...

watcher.start(logger, api, files, excludePatterns, arrify(cli.flags.source), process.stdin);

Again, just exploring/messing around. It "works" but the tests for watcher still fail. Will look into it this night.

novemberborn · 2016-02-10T11:13:46Z

Exporting files and excludePatterns could work but it'd require a big change in a lot of files (I think), so we'd have to get permission from the powers that be

Those patterns where hidden away inside a function before --watch landed so it should be OK. Real question is where they should be defined such that programmatic usage of the API is still possible even if the patterns are provided when instantiating the API or running the tests.

Again, just exploring/messing around. It "works" but the tests for watcher still fail. Will look into it this night.

Yup that should do it. Just need to provide compatible values in https://github.com/sindresorhus/ava/blob/bdb8b6ebaa05f1dc31cdab7c84aed7d92317b8ed/test/watcher.js#L23 and https://github.com/sindresorhus/ava/blob/bdb8b6ebaa05f1dc31cdab7c84aed7d92317b8ed/test/watcher.js#L95.

sotojuan · 2016-02-10T15:38:19Z

In test/watcher.js:

files = [
  'test.js',
  'test-*.js',
  'test'
];

excludePatterns = [
  '!**/node_modules/**',
  '!**/fixtures/**',
  '!**/helpers/**'
];

...

var start = function (sources) {
  subject.start(logger, api, files, excludePatterns, sources || [], stdin);
};

Under the "Chokidar is installed tests" I get not ok test unfinished for any I try:

test/watcher.js ....................................... 2/4
  chokidar is installed > watches for default source file changes, as well as test files
  not ok test unfinished: watches for default source file changes, as well as test files
    at:
      line: 128
      column: 19
      file: test/watcher.js
      function: test
    source: |
      pending.push(_t.test(name, fn));

  not ok missing plan
    results:
      ok: false
      count: 1
      pass: 1

novemberborn · 2016-02-10T16:19:00Z

@sotojuan hard to say. Could you push some code so I can debug it here?

sotojuan · 2016-02-10T18:49:49Z

Done, let me know if you can help!

novemberborn · 2016-02-11T17:42:28Z

@sotojuan just had to declare the variables and update more api.files and api.excludePatterns references: https://github.com/novemberborn/ava/commit/aa4e211268bff3ed386ce5c4579c0624ff3c4358.

novemberborn · 2016-02-11T17:48:15Z

I think you should keep the default excludePatterns in the API constructor. It only needs to be defined once since it's used for every run. Then the watcher can simply access it like before.

Still need to resolve whether we always want to prefix test titles (even if there's only a single test file) or only when tests are rerun via the watcher. How about making it an option on the API itself, and if --watch is enabled set it to true? That way the watcher output is consistent but regular output is still minimal, and we won't need a second argument to run() in order to differentiate.

sotojuan · 2016-02-11T18:31:22Z

I think you should keep the default excludePatterns in the API constructor. It only needs to be defined once since it's used for every run. Then the watcher can simply access it like before.

Sure, that makes sense.

Still need to resolve whether we always want to prefix test titles (even if there's only a single test file) or only when tests are rerun via the watcher. How about making it an option on the API itself, and if --watch is enabled set it to true? That way the watcher output is consistent but regular output is still minimal, and we won't need a second argument to run() in order to differentiate.

Also makes sense to me, doesn't seem that hard to implement. I'm interested in what the other mantainers think though. If they're on board with it, I'll work on it tonight.

// @sindresorhus @jamestalmage @vdemedes

sindresorhus · 2016-02-13T17:10:36Z

👍 Sounds good. I would still want minimal output in non-watch mode.

sotojuan · 2016-02-16T20:45:29Z

All right, I think I got it but I feel like I am missing something. Let me know!

novemberborn · 2016-02-17T11:05:03Z

api.js

+		this[key] = this[key].bind(this);
+	}, this);
+
+	this._reset();


If you look at the overall diff you'll see that these lines were moved above this.excludePatterns. Would be nice to leave them undisturbed.

novemberborn · 2016-02-17T11:16:48Z

All right, I think I got it but I feel like I am missing something. Let me know!

Getting close! The code is now doing a little bit of the old behavior, and a little bit of the new behavior.

cli.js should control the default files patterns. Api#run() should assume it's called with patterns and not force defaults. That means the watcher should call Api#run() with the files argument it received from cli.js.

explicitTitles is either on or off, it doesn't need to change based on the arguments to Api#run(). It's configured correctly by cli.js based on the --watch flag.

sotojuan · 2016-02-17T17:08:04Z

Okay, I'll give these a shot throughout the day. One thing I am confused about though, look at this test. Supposedly it's testing for prefixes, but it's a single file, so it fails with the changes since by default there's no explicit titles for just one file. Should I update the test so it sets the explicitTitles option? Or is it a problem with my changes?

It's the same case with a couple of the tests in api.js. I added explicitTitles: true to those that were failing and the changes you mentioned above and all tests are passing, but I'd like to know before pushing!

novemberborn · 2016-02-17T18:32:40Z

Should I update the test so it sets the explicitTitles option?

Yes.

It's the same case with a couple of the tests in api.js. I added explicitTitles: true to those that were failing and the changes you mentioned above and all tests are passing, but I'd like to know before pushing!

Push away! :)

sotojuan · 2016-02-17T19:33:05Z

Builds and tests pass fine in my Macbook, not sure what's up with AppVeyor.

novemberborn · 2016-02-18T11:06:37Z

test/watcher.js


 	try {
-		subject.start({}, {files: [], excludePatterns: []}, []);
+		subject.start({}, {}, [], []);


Would be nice to keep {exludePatterns: []} here, even though the test doesn't fail without it.

sotojuan · 2016-02-18T16:36:05Z

OK, well for now I fixed all the small stuff. Like I mentioned earlier, the test for failFast does need explicitTitles to be set to true as the output it tests for has prefixes.

I'm not very experienced with conflicts so any instructions on how to deal with that would be great!

novemberborn · 2016-02-18T16:54:53Z

Like I mentioned earlier, the test for failFast does need explicitTitles to be set to true as the output it tests for has prefixes.

Yes but if you check the full diff you'll notice you added those prefixes in this PR ;-)

novemberborn · 2016-02-18T17:03:02Z

test/watcher.js

+		'!**/node_modules/**',
+		'!**/fixtures/**',
+		'!**/helpers/**'
+	];


Need to remove this now.

novemberborn · 2016-02-18T17:05:25Z

I'm not very experienced with conflicts so any instructions on how to deal with that would be great!

https://help.github.com/articles/resolving-a-merge-conflict-from-the-command-line/ may help.

This will pull in the new watcher test in test/cli.js, so don't forget about https://github.com/novemberborn/ava/commit/651343f9d864b45aae187bad0958a908546a23f8 ;-)

sotojuan · 2016-02-18T17:06:18Z

Yes but if you check the full diff you'll notice you added those prefixes in this PR ;-)

Haha I totally forgot I did that! It's been a while!

sotojuan · 2016-02-23T20:00:56Z

@novemberborn When you are not busy, can you review? Thanks!

novemberborn · 2016-02-24T10:48:15Z

When you are not busy, can you review? Thanks!

Oh didn't notice you fixed the merge conflict. Next time ping me more explicitly when you've updated the PR 😄

The watcher test in test/cli.js is failing. The solution is in https://github.com/novemberborn/ava/commit/651343f9d864b45aae187bad0958a908546a23f8 (minus the console.error call I left in by mistake).

novemberborn · 2016-02-24T10:48:31Z

LGTM otherwise.

sotojuan · 2016-02-24T14:55:12Z

Thanks, um should I just copy what's in that commit you linked me to or is that something that will be merged?

novemberborn · 2016-02-24T14:56:20Z

@sotojuan copy yea. It only lives within my fork :)

novemberborn · 2016-02-24T15:39:10Z

@sotojuan are you running all tests?

sotojuan · 2016-02-24T15:39:43Z

I had some stuff I hadn't merged with master yet, should be good now.

sotojuan · 2016-02-24T15:44:36Z

Phew, all green on Travis (AppVeyor will take longer), finally!

novemberborn · 2016-02-24T15:45:39Z

Great! ❗ 🎉

sindresorhus · 2016-02-25T17:41:37Z

LGTM

vadimdemedes · 2016-02-25T19:55:57Z

LGTM!

novemberborn · 2016-02-26T12:16:23Z

Thanks @sotojuan!

sindresorhus · 2016-02-26T12:47:37Z

Thank you @sotojuan! Good work as usual.

avajs/ava#536

Move files argument to api.run()

985d5a1

Juan Soto added 2 commits February 10, 2016 11:49

Pass files and exclude patterns directly to watcher.start

c47e2e4

Update watcher test

86581d4

Juan Soto added 3 commits February 11, 2016 15:10

Move excludePatterns to API constructor

57c3de2

Update watcher.start to not take excludePatterns

f17d697

Fix watcher tests

ab0d2ae

Make explicitTitles an API option

f483408

novemberborn reviewed Feb 17, 2016
View reviewed changes

Clean up API and update tests

1d6d844

novemberborn reviewed Feb 18, 2016
View reviewed changes

Juan Soto added 4 commits February 18, 2016 10:58

Uncomment tests

90075f2

Remove event field in watcher tests

63f11be

Restore deleted API test

7a476b5

Add excludePatterns to dummy API in watcher test

4ffbc42

novemberborn reviewed Feb 18, 2016
View reviewed changes

test/watcher.js Outdated

'!**/node_modules/**',

'!**/fixtures/**',

'!**/helpers/**'

];

Copy link

Member

novemberborn Feb 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to remove this now.

Juan Soto added 2 commits February 18, 2016 12:05

Remove prefixes from fail-fast test

72651af

Cleanup watcher tests

10ed6a8

Juan Soto added 3 commits February 24, 2016 10:24

Update watcher and tests

9b2d29c

Merge branch 'master' into files-in-run

46e65d2

Update api tests

463fa85

novemberborn mentioned this pull request Feb 24, 2016

Only run tests for changed sources #544

Merged

novemberborn closed this in e26bd05 Feb 26, 2016

pine added a commit to taskrjs/fly-ava that referenced this pull request Mar 12, 2016

fix(Api): move files arguments

3bff9d4

avajs/ava#536

Move files argument to api.run() #536

Move files argument to api.run() #536

Uh oh!

Conversation

sotojuan commented Feb 9, 2016

Uh oh!

mention-bot commented Feb 9, 2016

Uh oh!

novemberborn commented Feb 9, 2016

Uh oh!

sotojuan commented Feb 9, 2016

Uh oh!

novemberborn commented Feb 10, 2016

Uh oh!

sotojuan commented Feb 10, 2016

Uh oh!

novemberborn commented Feb 10, 2016

Uh oh!

sotojuan commented Feb 10, 2016

Uh oh!

novemberborn commented Feb 11, 2016

Uh oh!

novemberborn commented Feb 11, 2016

Uh oh!

sotojuan commented Feb 11, 2016

Uh oh!

sindresorhus commented Feb 13, 2016

Uh oh!

sotojuan commented Feb 16, 2016

Uh oh!

novemberborn Feb 17, 2016

Choose a reason for hiding this comment

Uh oh!

novemberborn commented Feb 17, 2016

Uh oh!

sotojuan commented Feb 17, 2016

Uh oh!

novemberborn commented Feb 17, 2016

Uh oh!

sotojuan commented Feb 17, 2016

Uh oh!

novemberborn Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

sotojuan commented Feb 18, 2016

Uh oh!

novemberborn commented Feb 18, 2016

Uh oh!

novemberborn Feb 18, 2016

Choose a reason for hiding this comment

Uh oh!

novemberborn commented Feb 18, 2016

Uh oh!

sotojuan commented Feb 18, 2016

Uh oh!

sotojuan commented Feb 23, 2016

Uh oh!

novemberborn commented Feb 24, 2016

Uh oh!

novemberborn commented Feb 24, 2016

Uh oh!

sotojuan commented Feb 24, 2016

Uh oh!

novemberborn commented Feb 24, 2016

Uh oh!

novemberborn commented Feb 24, 2016

Uh oh!

sotojuan commented Feb 24, 2016

Uh oh!

sotojuan commented Feb 24, 2016

Uh oh!

novemberborn commented Feb 24, 2016

Uh oh!

sindresorhus commented Feb 25, 2016

Uh oh!

vadimdemedes commented Feb 25, 2016

Uh oh!

novemberborn commented Feb 26, 2016

Uh oh!

sindresorhus commented Feb 26, 2016

Move `files` argument to `api.run()` #536

Move `files` argument to `api.run()` #536