[FEATURE] Add fileExport capability #352

sap-sebelao · 2021-10-07T08:19:38Z

Some tools, such as Support Assistant, provide extra reports directly from tests
using a window._$files variable; this is no longer read anywhere and such reports are
currently unsupported. See
https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/core/support/RuleEngineOpaExtension.js#L243
this adds mechanism to read and save these the variable as files using a custom reporter
with some additions to browser.js to 'send' the results as part of karma.complete argument
unit tests included

JIRA: CPOUI5FOUNDATION-425

- Some tools, such as Support Assistant, provide extra reports directly from tests using a window._$files variable; this is no longer read anywhere and such reports are currently unsupported - See https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/core/support/RuleEngineOpaExtension.js#L243 - this adds mechanism to read and save these the variable as files using a custom reporter with some additions to browser.js to 'send' the results as part of karma.complete argument

CLAassistant · 2021-10-07T08:19:44Z

All committers have signed the CLA.

coveralls · 2021-10-07T08:37:48Z

Coverage increased (+2.3%) to 55.26% when pulling a1b96f1 on sap-sebelao:qunitCustomFiles into 6e50872 on SAP:master.

Improve mechanism of writing files to prevent overwriting of existing files.

README.md

KlattG

Just a few small suggestions. Otherwise LGTM

KlattG · 2021-11-30T16:20:01Z

README.md

+Type: `boolean` or `object`  
+Default: `false`
+
+Configures whether report files provided by tools like the UI5 Support Assistant are exported to the file system.  


Suggested change

Configures whether report files provided by tools like the UI5 Support Assistant are exported to the file system.

Configures whether report files provided by tools like UI5 Support Assistant are exported to the file system.

KlattG · 2021-11-30T16:20:55Z

README.md

+}
+```
+
+Projects can also add report files by itself by setting or enhancing the global `window._$files` array in the executed source code on the following way:


Suggested change

Projects can also add report files by itself by setting or enhancing the global `window._$files` array in the executed source code on the following way:

Projects can also add report files by themselves by setting or enhancing the global `window._$files` array in the executed source code in the following way:

KlattG · 2021-11-30T16:21:51Z

lib/errors.js

@@ -267,6 +267,35 @@ module.exports = function(config) {
 	});
 };`,

+		invalidFileExportReporterUsage: () => `error 21:


Suggested change

invalidFileExportReporterUsage: () => `error 21:

invalidFileExportReporterUsage: () => `Error 21:

I did not take over this suggestion to be consistent to the existing error messages

Fine, but could we have a central resources file (like i18n) where all error messages are stored and can be easily modified? Then we could easily fix such things without having to sacrifice consistency.

KlattG · 2021-11-30T16:43:39Z

lib/errors.js

@@ -267,6 +267,35 @@ module.exports = function(config) {
 	});
 };`,

+		invalidFileExportReporterUsage: () => `error 21:


Fine, but could we have a central resources file (like i18n) where all error messages are stored and can be easily modified? Then we could easily fix such things without having to sacrifice consistency.

RandomByte · 2021-11-30T17:31:47Z

lib/client/browser.js

+							if (config.fileExport) {
+								const testPageName = getTestPageName(qunitHtmlFile);
+								(testWindow.contentWindow._$files || []).forEach((file) => {
+									file.name = `TEST-${testPageName}-FILE-${file.name}`;


Is this how it was done in Selenium? I.e. with the strings "TEST" and "FILE"?

Yes, this format is taken from the java implementation

RandomByte · 2021-11-30T17:38:49Z

lib/fileExportReporter.js

+	}
+
+	const fileExtension = path.extname(fileName);
+	const fileNameWithoutExtension = fileName.slice(0, fileName.lastIndexOf(fileExtension));


Alternative:

Suggested change

const fileNameWithoutExtension = fileName.slice(0, fileName.lastIndexOf(fileExtension));

const fileNameWithoutExtension = path.basename(fileName, fileExtension);

See https://nodejs.org/api/path.html#pathbasenamepath-ext

But I wouldn't mind the current solution as well 🤷

Good idea but not an alternative because path.basename would trim the starting directory path

RandomByte · 2021-11-30T17:50:48Z

lib/fileExportReporter.js

+		}
+
+		for (const file of result.exportFiles) {
+			const pathSegments = [outputDir, file.name];


Suggested change

const pathSegments = [outputDir, file.name];

const pathSegments = [outputDir, path.basename(file.name)];

"sanitize" file name to ensure it is really only a file name and not a path.

RandomByte · 2021-11-30T17:51:19Z

lib/fileExportReporter.js

+		for (const file of result.exportFiles) {
+			const pathSegments = [outputDir, file.name];
+			if (multiBrowsers) {
+				pathSegments.splice(1, 0, browser.name);


Suggested change

pathSegments.splice(1, 0, browser.name);

pathSegments.splice(1, 0, path.basename(browser.name));

Same for browser name I guess

RandomByte · 2021-11-30T17:57:51Z

lib/fileExportReporter.js

+		outputDir = defaultPath;
+	}
+
+	outputDir = helper.normalizeWinPath(path.resolve(config.basePath, outputDir));


Not sure, but I think normalization should happen before any platform specific path-action?

path.resolve won't actually resolve two Windows paths on a POSIX system, it will just append them. This might not be expected since on Windows the paths will actually be resolved in that case.

We need to normalize the configuration since it can be POSIX or Windows or both. Later when we path.join the file path, it will automatically be transformed into the right format for the current platform.

Maybe:

Suggested change

outputDir = helper.normalizeWinPath(path.resolve(config.basePath, outputDir));

outputDir = path.posix.resolve(helper.normalizeWinPath(config.basePath), helper.normalizeWinPath(outputDir));

As discussed, a simple path.join should be sufficient since we already expect in browser.js the basePath to be in posix format.

sap-sebelao · 2021-12-01T08:16:29Z

I looked at the changes and also retested this manually - everything seems to be still working as required for further processing of the files, the naming changes did not cause any unexpected trouble so far. Great job!

I will continue watching this and give feedback if required, but it seems like this is in good hands now. As the original committer I can't explicitly 'approve' this, so have at least a 👍 from me.

matz3 · 2021-12-02T14:56:35Z

lib/fileExportReporter.js

+	baseReporterDecorator(this);
+
+	async function writeSingleFile(outputFile, content) {
+		log.info(`Writing file: ${outputFile}`);


This doesn't output the final path, as getUniqueFileName is called after.
Also, I think it would be sufficient to only have one "info" log per file, so this log should rather become "debug".

matz3 · 2021-12-02T15:06:16Z

lib/fileExportReporter.js

+			return;
+		}
+
+		for (const file of result.exportFiles) {


Do we want to add some error handling here?
Not only in case exportFiles is not an array, but also when name or content are undefined or define an unexpected type.
Currently, no name causes "undefined" to be used as name, and if content is not of type string, it causes an exception. I'd suggest to check for type string and log a warning if a file is not written.

You might check out the Java implementation. Just search for LOGGER.severe("Invalid file object. \"name\" and \"content\" must be strings

matz3 · 2021-12-02T15:22:11Z

lib/fileExportReporter.js

+		}
+
+		for (const file of result.exportFiles) {
+			const pathSegments = [outputDir, path.basename(file.name)];


In Java we escaped the name to remove special characters and slashes:

result = result.replaceAll("[:*?\"<>|]", ""); result = result.replaceAll("[\\\\/]", ".");

Not sure whether we should just stick with that.

path.basename will still cause strange behavior when using ...
My question would be whether we want to make this "safe" so that it can't be misused or just that strange file names don't cause errors.

matz3 · 2021-12-02T15:24:44Z

lib/fileExportReporter.js

+		for (const file of result.exportFiles) {
+			const pathSegments = [outputDir, path.basename(file.name)];
+			if (multiBrowsers) {
+				pathSegments.splice(1, 0, path.basename(browser.name));


for browser.name, we could use the behavior as karma-coverage does: https://github.com/karma-runner/karma-coverage/blob/4adb3efaa3ff0e709da91a3d6eb86e0c66a6e3cd/lib/reporter.js#L179

Done, is covered now by escapeFileName

matz3 · 2021-12-07T14:48:20Z

lib/fileExportReporter.js

+				await writeSingleFile(exportPath, escapeFileName(file.name), file.content);
+			}
+		} catch (err) {
+			log.warn("An unexpected error occured while exporting files\n\t" + err.message);


if we set exit code 1, we should also log this as an error

matz3 · 2021-12-07T14:48:50Z

lib/fileExportReporter.js

+		if (multiBrowsers) {
+			exportPath = path.join(exportPath, escapeFileName(browser.name));
+		}
+		try {


try-catch should be around the whole function body (onBrowserComplete). Otherwise errors won't be handled.

My review has been addressed

matz3

👍🏻

sap-sebelao changed the title ~~Add custom files read-send mechanism on complete~~ Add custom files read-save mechanism on complete Oct 7, 2021

sap-sebelao added 2 commits October 7, 2021 10:29

Fix eslint errors

dafe9ff

Fix test expectation path separator

adf2211

matz3 self-requested a review October 7, 2021 11:41

matz3 added the enhancement label Oct 7, 2021

larskissel added 4 commits November 26, 2021 14:25

Add script mode and integration tests

2a00155

Improve mechanism of writing files to prevent overwriting of existing files.

Fix broken test

4979c3c

Fix broken test (win/unix path)

c909a51

Fix: Reporter waits for the export to be completed

67be8b5

matz3 reviewed Nov 30, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Improve fileExport documentation

176e950

larskissel requested a review from KlattG November 30, 2021 15:37

KlattG suggested changes Nov 30, 2021

View reviewed changes

Adapt documentation as suggested

0fbc83b

KlattG previously approved these changes Nov 30, 2021

View reviewed changes

RandomByte reviewed Nov 30, 2021

View reviewed changes

RandomByte previously requested changes Nov 30, 2021

View reviewed changes

Improve export path handling as suggested in review

109448f

larskissel dismissed KlattG’s stale review via 109448f December 1, 2021 13:43

larskissel requested a review from matz3 December 1, 2021 13:43

matz3 requested changes Dec 2, 2021

View reviewed changes

Improve error handling

603a4fa

matz3 reviewed Dec 7, 2021

View reviewed changes

Change log level for unexpected errors

a1b96f1

larskissel requested a review from matz3 December 7, 2021 15:16

matz3 approved these changes Dec 7, 2021

View reviewed changes

larskissel changed the title ~~Add custom files read-save mechanism on complete~~ [FEATURE] Add fileExport capability Dec 8, 2021

larskissel merged commit 56aac75 into SAP:master Dec 8, 2021

	Configures whether report files provided by tools like the UI5 Support Assistant are exported to the file system.
	Configures whether report files provided by tools like UI5 Support Assistant are exported to the file system.

	Projects can also add report files by itself by setting or enhancing the global `window._$files` array in the executed source code on the following way:
	Projects can also add report files by themselves by setting or enhancing the global `window._$files` array in the executed source code in the following way:

	invalidFileExportReporterUsage: () => `error 21:
	invalidFileExportReporterUsage: () => `Error 21:

	const fileNameWithoutExtension = fileName.slice(0, fileName.lastIndexOf(fileExtension));
	const fileNameWithoutExtension = path.basename(fileName, fileExtension);

	const pathSegments = [outputDir, file.name];
	const pathSegments = [outputDir, path.basename(file.name)];

	pathSegments.splice(1, 0, browser.name);
	pathSegments.splice(1, 0, path.basename(browser.name));

	outputDir = helper.normalizeWinPath(path.resolve(config.basePath, outputDir));
	outputDir = path.posix.resolve(helper.normalizeWinPath(config.basePath), helper.normalizeWinPath(outputDir));

[FEATURE] Add fileExport capability #352

[FEATURE] Add fileExport capability #352

Conversation

sap-sebelao commented Oct 7, 2021 • edited by larskissel Loading

CLAassistant commented Oct 7, 2021 • edited by cla-assistant bot Loading

coveralls commented Oct 7, 2021 • edited Loading

KlattG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sap-sebelao commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matz3 left a comment

Choose a reason for hiding this comment

sap-sebelao commented Oct 7, 2021 •

edited by larskissel

Loading

CLAassistant commented Oct 7, 2021 •

edited by cla-assistant bot

Loading

coveralls commented Oct 7, 2021 •

edited

Loading