Add ability to run a test residing on a given line #2181

qti3e · 2019-07-08T14:39:01Z

🦄 Fixes: #2138

There are just a few edges case that I found it useful to have your comments on :)

Currently, negative numbers are just ignored, maybe throw an error or something?
What should happen if the :... is actually part of the file name, (test.js:5 a valid filename on Linux at least)!?
When running in the watch mode, the line number might change and results in different tests to be executed.
Due to the implementation using line numbers that are higher than the actual number of lines in the file will result in the last test being executed, it would be nice to have a way to get line numbers in the file without the overhead of reading the file from disk.

Also, there is definitely room for tests to be improved, I couldn't actually get very useful information from runStatus.stats for what I had in mind, (name of executed tests) if the current test is not enough I'd be happy to hear about a way to get that information from the API.

IssueHunt Summary

sindresorhus · 2019-07-09T08:28:07Z

Negative numbers should throw a user-friendly error.

sindresorhus · 2019-07-09T08:29:33Z

While test.js:5 might be a valid file name on Linux, it’s not something anyone would actually use. It’s not a concern.

sindresorhus · 2019-07-09T08:31:23Z

The range syntax should work for the files setting in the AVA config too. I don’t see that documented.

sindresorhus · 2019-07-09T08:32:50Z

I’m not sure it makes sense at all to allow the range syntax when using watch.

qti3e · 2019-07-11T09:24:14Z

@sindresorhus I guess everything works as expected now, I moved the whole parser logic to a new file, and also added the support for ranges in glob as well as in the files argument in API.run() so that this range syntax thing works everywhere the same way (configs, CLI, etc.)

That being said now we can expect a glob pattern such as test-*.js:5 to work just fine.

There is only one option added to the runner named lines, which determines which lines are expected to be executed (an empty array means every line).

There is no negative number allowed in a range string, which means test.js:-4 is treated as a file with the name test.js:-4 rather than line -4 in test.js and well that leads to a file not found status, which I believe is best to be addressed as part of #2158 (which I'm going to take right after this PR if that's ok), and I'd rather avoid doing this error handling as part of this PR to keep it as simple as it is and keep it clean.

/c.c @novemberborn

novemberborn

Hi @qti3e, thanks for the PR!

The range syntax should work for the files setting in the AVA config too. I don’t see that documented.

@sindresorhus I don't see how this is useful. If you want to always run part of a test file, make a new test file. Our globbing logic is still really complex and I don't think we should add more complexity to it.

I’m not sure it makes sense at all to allow the range syntax when using watch.

Agreed. Another argument for not allowing it in the configuration. Then, we can enforce this in lib/cli.js by looking for watch mode and the presence of ranges in the provided files.

Having reviewed the PR it definitely looks easier to implement and maintain if we only support this syntax on the CLI.

… that leads to a file not found status, which I believe is best to be addressed as part of #2158 (which I'm going to take right after this PR if that's ok)

@qti3e see #2184.

Due to the implementation using line numbers that are higher than the actual number of lines in the file will result in the last test being executed, it would be nice to have a way to get line numbers in the file without the overhead of reading the file from disk.

If we restrict this feature to files passed on the CLI, it's definitely legit to parse the file counting its lines.

novemberborn · 2019-07-14T13:44:55Z

lib/range.js

+	for (const file of files) {
+		const [actualFilename, rangeStr] = splitRangeStr(file);
+
+		if (any.has(actualFilename)) {


Did you consider merging the ranges?

novemberborn · 2019-07-14T13:45:31Z

lib/range.js

+
+function parseRange(files) {
+	const any = new Set();
+	const ranges = {};


Let's use a Map instead.

novemberborn · 2019-07-14T13:46:09Z

lib/range.js

+const union = require('array-union');
+
+function parseRange(files) {
+	const any = new Set();


What does any mean? I can't quite tell from the logic below.

@nov parseRange should work on both ranged paths and normal paths, imagine we have an input like ['a.js', 'a.js:4'] in this case the a.js path has a higher priority and should cancel out a.js, this set is used to store file paths that do not have a range syntax in them.

I see. Should that maybe print an error instead? Or perhaps we could run only the selected lines and print a warning "ignoring other tests from a.js since you selected lines" (well not quite that, but you get the idea).

lib/range.js

novemberborn · 2019-07-14T13:49:06Z

lib/range.js

+		const lines = rangeParser.parse(rangeStr);
+
+		if (ranges[actualFilename]) {
+			ranges[actualFilename] = union(ranges[actualFilename], lines);


Rather than using array-union you could concatenate the arrays, and then pass them through a [...newSet(lines)] at the very end to get unique values out.

novemberborn · 2019-07-14T13:58:17Z

lib/range.js

+			continue;
+		}
+
+		const lines = rangeParser.parse(rangeStr);


Why isn't the range string parsed in splitRangeStr()?

That function could be rename to parseFileSelection(), returning a file and extracted lines, and then this parseRange() function could be renamed to processFileSelections() or something. Maybe rename the range module too.

Because of how this implementation handles globs I needed to have the range part as a string, and of course, we need another function to parse that part.

Right, so this will be a lot simpler without globs 😄

novemberborn · 2019-07-14T14:03:40Z

lib/runner.js

+				for (const jsCallsite of callsites()) {
+					const callsite = sourceMapSupport.wrapCallSite(jsCallsite);
+					const fileName = callsite.getFileName();
+					if (fileName === this.file) {


I imagine this will break when using AVA with pre-compiled test files. The selected file may be build/test.js but source-map-support may translate the callsite to src/test.js. You should check the files before the source map is applied.

This also needs a comment to explain why we need to check the files in the first place (because the test() may be invoked in a helper file — which is its own edge case that I wouldn't know how to deal with).

We can't check the file before the source map is applied after the translation line numbers are all changed, (a.js:5 might end up running line 6.)

We can't check the file before the source map is applied after the translation line numbers are all changed, (a.js:5 might end up running line 6.)

Yes I get that. My point though is that applying the source map may change the fileName of the call site.

novemberborn · 2019-07-14T14:09:57Z

lib/runner.js

+					// get the last line of the previous test.
+					const range = [lastCallsiteLineNumber, callsiteLineNumber - 1];
+					const includedInRange = this.lines.some(n => n >= range[0] && n <= range[1]);
+					lastMetadata.exclusive = this.match.length === 0 ?


exclusive will be true if the test is declared using .only(). But, crucially, tests can also be skipped.

I think the ranges should narrow down from the initial test selection. If there are exclusive tests, then only those that match the ranges should be run. If there are no exclusive tests, then skipped tests should never be run.

novemberborn · 2019-07-14T14:10:35Z

lib/runner.js

+				}
+
+				lastCallsiteLineNumber = callsiteLineNumber;
+				lastMetadata = metadata;


Rather than tracking the last metedata object and callsite, perhaps you could add the callsite number to the metadata, and then adjust the test selection once all tests have been declared?

novemberborn · 2019-07-14T14:11:22Z

lib/runner.js

+
+				if (lastMetadata) {
+					// `callsiteLineNumber` is the beginning of the current test, -1 to
+					// get the last line of the previous test.


This does include whitespace, but I think that's OK for now.

I don't think there is a way to handle this case unless you parse the source file to it's AST and then extract when the function call actually ends (this information is not in the callsite data.)

qti3e · 2019-07-18T21:16:12Z

@novemberborn Please let me know your final decision about dropping ranged globs.
(Because now it's done and it works really well, anyway just let me know and I'll drop it.)

sindresorhus · 2019-07-21T08:28:08Z

I don't see how this is useful. If you want to always run part of a test file, make a new test file. Our globbing logic is still really complex and I don't think we should add more complexity to it.

I agree. I didn't think that through when first commenting.

Having reviewed the PR it definitely looks easier to implement and maintain if we only support this syntax on the CLI.

👍

sindresorhus · 2019-07-21T08:29:29Z

There is no negative number allowed in a range string, which means test.js:-4 is treated as a file with the name test.js:-4 rather than line -4 in test.js and well that leads to a file not found status

I think that should be handled here. It's clearly not a valid filename, so we should give the user a helpful and relevant error message that it's an invalid range.

qti3e · 2019-07-26T07:18:57Z

@novemberborn PTAL, also please restart that Travis task.

novemberborn

Very nice, I think we're getting there @qti3e!

There is no negative number allowed in a range string, which means test.js:-4 is treated as a file with the name test.js:-4 rather than line -4 in test.js and well that leads to a file not found status

I think that should be handled here. It's clearly not a valid filename, so we should give the user a helpful and relevant error message that it's an invalid range.

I agree with this.

novemberborn · 2019-07-28T14:50:21Z

docs/05-command-line.md

+10 });
+```
+
+> Please note that using any number more than the maximum line-number will execute the last test.


Don't make this a quote. Use italics instead:

Note that ranges beyond the last line of the file will select the last test.

novemberborn · 2019-07-28T14:51:18Z

docs/05-command-line.md

+
+> Please note that using any number more than the maximum line-number will execute the last test.
+
+Using `--match` beside this feature will only match the tests on the given area.


How about:

You can combine line ranges with the --match option.

novemberborn · 2019-07-28T14:58:03Z

lib/range.js

+			continue;
+		}
+
+		ranges.set(actualFilename, ranges.get(actualFilename).concat(lines));


We now need to set the first value or otherwise modify the arary:

if (!ranges.has(actualFilename)) { ranges.set(actualFilename, lines) } else { ranges.get(actualFileName).push(...lines) }

novemberborn · 2019-07-28T15:00:22Z

lib/range.js

+function parseFileSelections(files) {
+	const ranges = new Map();
+	const ignored = [];
+	const filenames = [];


Let's make this a set, too.

novemberborn · 2019-07-28T15:00:47Z

lib/range.js

+	const filenames = [];
+
+	for (const file of files) {
+		const [actualFilename, lines] = parseFileSelection(file);


With filenames a set, you can add the actualFilename to it here, regardless of any other condition.

novemberborn · 2019-07-28T15:21:22Z

lib/runner.js

+				for (const jsCallsite of callsites()) {
+					const callsite = sourceMapSupport.wrapCallSite(jsCallsite);
+					const fileName = callsite.getFileName();
+					if (jsCallsite === this.file || fileName === this.file) {


Did you mean jsCallsite.getFileName()?

I don't think this needs to be compared to the source-map converted filename. You can't pass those on the CLI since you need to provide the file that AVA can actually load.

novemberborn · 2019-07-28T15:21:55Z

lib/runner.js

+			if (this.lines.length > 0) {
+				let callsiteLineNumber;
+
+				for (const jsCallsite of callsites()) {


Let's rename jsCallsite to callsite.

novemberborn · 2019-07-28T15:22:01Z

lib/runner.js

+				let callsiteLineNumber;
+
+				for (const jsCallsite of callsites()) {
+					const callsite = sourceMapSupport.wrapCallSite(jsCallsite);


And rename this to wrapped.

novemberborn · 2019-07-28T15:26:39Z

lib/runner.js

 				scheduledStart = true;
 				process.nextTick(() => {
+					this.callsiteLineNumbers.push(Infinity);
+					this.callsiteLineNumbers.sort((a, b) => a - b);


Let's do this work inside the start() method instead.

We should deduplicate the line numbers as well. It's an edge case but you could specify two tests on the same line.

novemberborn · 2019-07-28T15:29:54Z

lib/api.js

+			const filename = path.resolve(this.options.resolveTestsFrom, file);
+			ranges.set(filename, ranges.get(file));
+			return filename;
+		});


Why not do this processing in the CLI instead?

novemberborn · 2019-11-03T17:43:16Z

Closing due to inactivity. @qti3e I do hope you'll find the time to come back to this!

qti3e force-pushed the q/ln branch 2 times, most recently from a48a3b5 to 561d222 Compare July 11, 2019 08:54

novemberborn requested changes Jul 14, 2019

View reviewed changes

Add ability to run a test residing on a given line

ca7629e

qti3e force-pushed the q/ln branch from 561d222 to ca7629e Compare July 23, 2019 11:50

novemberborn requested changes Jul 28, 2019

View reviewed changes

novemberborn closed this Nov 3, 2019

novemberborn mentioned this pull request Nov 10, 2019

Add ability to run a test residing on a given line #2138

Closed


		> Please note that using any number more than the maximum line-number will execute the last test.

		Using `--match` beside this feature will only match the tests on the given area.

Add ability to run a test residing on a given line #2181

Add ability to run a test residing on a given line #2181

Uh oh!

Conversation

qti3e commented Jul 8, 2019 • edited by issuehunt-oss bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sindresorhus commented Jul 9, 2019

Uh oh!

sindresorhus commented Jul 9, 2019

Uh oh!

sindresorhus commented Jul 9, 2019

Uh oh!

sindresorhus commented Jul 9, 2019

Uh oh!

qti3e commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qti3e commented Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sindresorhus commented Jul 21, 2019

Uh oh!

sindresorhus commented Jul 21, 2019

Uh oh!

qti3e commented Jul 26, 2019

Uh oh!

novemberborn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qti3e commented Jul 8, 2019 •

edited by issuehunt-oss bot

Loading

qti3e commented Jul 11, 2019 •

edited

Loading

qti3e commented Jul 18, 2019 •

edited

Loading