Fix for not showing path for error messages on windows #316

mkls · 2016-08-31T21:35:37Z

This bug has been around for a while now, there are two open PR's (#221 and #295) but no activity recently.
This is basically the solution proposed by @hgwood.
Tried to keep changes to the bare minimum.

hgwood · 2016-08-31T21:50:24Z

Third time's the charm!

ljharb · 2016-08-31T21:56:44Z

lib/test.js

@@ -217,7 +217,7 @@ Test.prototype._assert = function assert (ok, opts) {
    if (!ok) {
        var e = new Error('exception');
        var err = (e.stack || '').split('\n');
-        var dir = path.dirname(__dirname) + '/';
+        var dir = path.dirname(__dirname) + path.sep;


please use path.join here instead of directly referencing path.sep

Do you mean path.join(path.dirname(__dirname), path.sep) ?

Ah, I see that path.join will always omit the trailing separator. This is fine as-is.

ljharb · 2016-08-31T22:19:20Z

Are there tests that ensure this isn't breaking anything, and tests that are currently failing on windows?

mkls · 2016-08-31T23:47:39Z

As far as I can tell there were no specific tests for this feature, but this modification does not break the current tests. (On windows one test was breaking even before this, I can try to fix that if needed).

If a well tested, multi-platform stack-trace parser is desired, then we should probably include stacktrace-parser as suggested in #295.
I feel like a larger refactor would introduce more possibilities for errors and the current minimal solution is easier to inspect and verify on code review. Basically you only have to decide if the changed regex would still work. I could not come up with cases where it would not.

I will be on vacation from tomorrow, but if you could give me some guidance which way to go about it, I will try to do what it takes next week.

bsuh · 2016-09-06T00:46:50Z

I'd like to see proper errors in all cases. So I modified tape to console.log a stack trace as well as __dirname.

node index.js

	Stack Trace Path	__dirname
*nix	/Users/bsuh/tape-errors/node_modules/tape/lib/test.js	/Users/bsuh/tape-errors/node_modules/tape/lib
Windows	\vmware-host\Shared Folders\vagrant\tape-errors\node_modules\tape\lib\test.js	\vmware-host\Shared Folders\vagrant\tape-errors\node_modules\tape\lib

browserify -d index.js | tape-run

	Stack Trace Path	__dirname
*nix	node_modules/tape/lib/test.js	/node_modules/tape/lib
Windows	vmware-host/Shared Folders/vagrant/tape-errors/node_modules/tape/lib/test.js	/vmware-host\Shared Folders\vagrant\tape-errors\node_modules\tape\lib

As I see it there are 3 problems

proper errors with browserify is broken, because browserify insists on prefixing __dirname with '/' on all platforms and Chrome's stack traces don't contain prefix '/' in the filename.
tape's regexp for getting the filename from stack trace expects filename to begin with '/' not working with Windows filename.
Chrome's stack traces contain Unix style paths even on Windows, while browserify uses platform dependent paths for __dirname (while still prefixing with '/' on Windows ❗❓ ).

ljharb · 2016-09-06T01:20:29Z

@bsuh seems like a couple of those need filing upstream on browserify itself - can you file those, and link them here?

bsuh · 2016-09-06T03:04:20Z

@ljharb

Removing of'/' prefix from browserify __dirname seems like a breaking change. I think it's more practical for tape to handle it.
There is already a pull request to normalize __dirname/__filename to Unix style on Windows Fix some more Windows paths issues browserify/insert-module-globals#63 Not sure the likelyhood of it actually getting merged, since it's been open nearly a year.

hgwood · 2016-09-06T08:41:31Z

Lets be realistic. This is the third PR of this subject. The first two were never merged because one the two parties (the contributor or the maintainer) stopped answering requests/questions from the other party, after the discussion got too long (and that's OK).

The goal of this third attempt is to introduce the minimal change to fix the bug for most cases. The goal is not to fix for all cases (see #295 for that @bsuh).

Merging this PR is safe. The new regex is guaranteed to match anything the old one matched, so regressions are not possible. The worst than can happen is users that did not see any stack trace before seeing a partial stack trace. In the meantime, Windows users finally get proper stack traces on errors.

Lets get 80% of the benefits for 20% of the effort. 100% correctness can come later, one step at a time. :)

ljharb · 2016-09-06T22:36:55Z

@mkls would you mind rebasing on latest master so there's no merge commits?

I'll merge this after that.

This bug has been around for a while now. Tried to keep changes to the bare minimum.

- [Fix] `throws`: only reassign “message” when it is not already non-enumerable (#320) - [Fix] show path for error messages on windows (#316) - [Fix] `.only` should not run multiple tests with the same name (#299, #303) - [Deps] update `glob`, `inherits` - [Dev Deps] update `concat-stream`, `tap`, `tap-parser`, `falafel` - [Tests] [Dev Deps] Update to latest version of devDependencies tap (v7) and tap-parser (v2) (#318) - [Tests] ensure the max_listeners test has passing output - [Docs] improvements (#298, #317)

ljharb reviewed Aug 31, 2016
View reviewed changes

mkls added 2 commits September 7, 2016 01:20

Fix for not showing path for error messages on windows

7536fba

This bug has been around for a while now. Tried to keep changes to the bare minimum.

Fix for unit tests on windows

baca83c

ljharb merged commit d11bb14 into tape-testing:master Sep 6, 2016

This was referenced Sep 6, 2016

fixed windows path support for error messages #221

Closed

Fix Windows path support in error messages #295

Closed

No line number provided in Windows on failed tests (and proposed fix) #220

Closed

mkls mentioned this pull request Oct 17, 2016

Show failure location even if windows driver letter is lowercase #329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for not showing path for error messages on windows #316

Fix for not showing path for error messages on windows #316

mkls commented Aug 31, 2016

hgwood commented Aug 31, 2016

ljharb Aug 31, 2016

hgwood Aug 31, 2016

ljharb Aug 31, 2016

ljharb commented Aug 31, 2016

mkls commented Aug 31, 2016

bsuh commented Sep 6, 2016 •

edited

Loading

ljharb commented Sep 6, 2016

bsuh commented Sep 6, 2016

hgwood commented Sep 6, 2016 •

edited

Loading

ljharb commented Sep 6, 2016

Fix for not showing path for error messages on windows #316

Fix for not showing path for error messages on windows #316

Conversation

mkls commented Aug 31, 2016

hgwood commented Aug 31, 2016

ljharb Aug 31, 2016

Choose a reason for hiding this comment

hgwood Aug 31, 2016

Choose a reason for hiding this comment

ljharb Aug 31, 2016

Choose a reason for hiding this comment

ljharb commented Aug 31, 2016

mkls commented Aug 31, 2016

bsuh commented Sep 6, 2016 • edited Loading

node index.js

browserify -d index.js | tape-run

ljharb commented Sep 6, 2016

bsuh commented Sep 6, 2016

hgwood commented Sep 6, 2016 • edited Loading

ljharb commented Sep 6, 2016

bsuh commented Sep 6, 2016 •

edited

Loading

hgwood commented Sep 6, 2016 •

edited

Loading