Skip to content

test: make toErrorSnapshot windows compatible#503

Merged
karfau merged 4 commits into
masterfrom
tests-windows-compatible
Jul 1, 2023
Merged

test: make toErrorSnapshot windows compatible#503
karfau merged 4 commits into
masterfrom
tests-windows-compatible

Conversation

@karfau

@karfau karfau commented Jul 1, 2023

Copy link
Copy Markdown
Member

@shunkica told me in #498 that toErrorSnapshot doesn't work on windows

  • cater for windows backslash in file paths
  • handle windows backslash before passing it to regular expressions
  • convert to unix delimiter before returning the value for OS independent snapshots
  • refactor to use understandable variable names for each step

@karfau karfau requested a review from shunkica July 1, 2023 04:33
@shunkica

shunkica commented Jul 1, 2023

Copy link
Copy Markdown
Collaborator

On Windows libFile will have \ as the delimiter, and when you put it in regex, it will treat that as an escape character instead of a literal \

Even when that is replaced with \\ there are some new issues.

For example:

Snapshot name: `WF_EntityDeclared with mimeType text/xml should escalate Error thrown in onError to ParseError 1`

- Snapshot  - 1
+ Received  + 1

  Array [
    "error: entity not found:&e;
-     at replace (lib/sax.js:#0)",
+     at replace (lib\\sax.js:70)",
  ]

In any case it is very hard to debug this chain. Would you consider splitting it up?

@karfau

karfau commented Jul 1, 2023

Copy link
Copy Markdown
Member Author

I will try to find a solution for it.
Not really sure what you mean by "splitting it up".
If you have any suggestions code wise, feel free to make suggestions here or file a PR against this branch.

@shunkica

shunkica commented Jul 1, 2023

Copy link
Copy Markdown
Collaborator

I will try to find a solution for it. Not really sure what you mean by "splitting it up". If you have any suggestions code wise, feel free to make suggestions here or file a PR against this branch.

I can't (figure out) how to debug the chain output after every step. What I meant was splitting the chain up into assignments, eg:

	const errorMessage = error.message.replace(/([\n\r]+\s*)/g, '||');
	const errorStack = error.stack.split(/[\n\r]+/);
	const filteredErrorStack = errorStack.filter((l) => libFileMatch.test(l))[0];
        ...

@karfau

karfau commented Jul 1, 2023

Copy link
Copy Markdown
Member Author

Feel free to do that locally to figure out what is going on.
and if you think it's helpful in general, we can also include that into this PR.
I agree that this a is a part of the tests that is anyways quite hard to grasp, so maybe a little bit less functional programming style will help.

@shunkica

shunkica commented Jul 1, 2023

Copy link
Copy Markdown
Collaborator

This seems to work for me

function toErrorSnapshot(error, libFile) {
	// Escape the backslash for Windows paths and make the regex platform independent
	const escapedLibFile = libFile.replace(/\\/g, '\\\\');
	const libFileMatch = new RegExp(`[^(]*(${escapedLibFile})`);

	const errorMessage = error.message.replace(/([\n\r]+\s*)/g, '||');
	const splitErrorStack = error.stack.split(/[\n\r]+/);
	const firstLibFileLine = splitErrorStack.filter((l) => libFileMatch.test(l))[0];
	const strippedPath = firstLibFileLine.replace(libFileMatch, '$1');
	const strippedPos = strippedPath.replace(/:\d+\)$/, ')');

	const replacedFileAndLine = strippedPos.replace(new RegExp(`${escapedLibFile}:\\d+`), (fileAndLine) => {
		// replace separators in file path to '/' for consistent error snapshot
		const standardizedLibFile = libFile.replace(/\\/g, '/');
		return `${standardizedLibFile}:#${fileAndLine in LINE_TO_ERROR_INDEX ? LINE_TO_ERROR_INDEX[fileAndLine].index : -1}`;
	});

	return `${errorMessage}\n${replacedFileAndLine}`;
}

@karfau karfau changed the title test: make tests windows compatible test: make toErrorSnapshot windows compatible Jul 1, 2023
@karfau

karfau commented Jul 1, 2023

Copy link
Copy Markdown
Member Author

@shunkica I took your code snippet and tweaked a bit.
Can you confirm that the test still works for you on windows?

If you are joining as a maintainer we will add a windows run to the Github action workflow, so it doesn't break again :)

@shunkica shunkica left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants