Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Add fileExport capability #352

Merged
merged 12 commits into from
Dec 8, 2021
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,4 @@ deploy_key

# Custom directories
dist/
test/integration/*/karma-ui5-reports*
32 changes: 32 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,38 @@ ui5: {
}
```

### fileExport
Type: `boolean` or `object`
Default: `false`

Configures whether report files provided by tools like UI5 Support Assistant are exported to the file system.
Optionally, an output directory can be set to specify the export path.

Example `boolean`:
```js
ui5: {
fileExport: true
}
```

Example `object`:
```js
ui5: {
fileExport: {
outputDir: "directory/to/export/files"
}
}
```

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:
```js
window._$files = window._$files || [];
window._$files.push({
name: "file_name.txt",
content: "file content"
});
```

## API

### helper
Expand Down
58 changes: 57 additions & 1 deletion lib/client/browser.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,55 @@
const istanbulLibCoverage = require("istanbul-lib-coverage");
require("./discovery.js");

const patternTestResources = /(?:^|[^?#]*\/)(?:test-)?(?:resources|webapp)\/(.*)/;
const patternGenericTeststarter = /([^?#]+)\.qunit\.html\?testsuite=test-resources\/((?:[^/]+\/)*testsuite(?:[.a-z0-9]+)?\.qunit)&test=([^&]+)(?:&|$)/;
const patternSpecificTeststarter = /([^?#]+)\.qunit\.html\?test=([^&]+)(?:&|$)/;
const patternAnyTest = /([^?#]*)[?#](?:.*)/;

function getTestPageName(qunitHtmlFile) {
let matches;
let result;

matches = qunitHtmlFile.match(patternTestResources);
if (matches) {
qunitHtmlFile = matches[1];
}

matches = qunitHtmlFile.match(patternGenericTeststarter);
if (matches) {
result = matches[2] + "--" + matches[3];
}

if (!result) {
matches = qunitHtmlFile.match(patternSpecificTeststarter);
if (matches) {
result = matches[1] + "--" + matches[2];
}
}

if (!result) {
matches = qunitHtmlFile.match(patternAnyTest);
if (matches) {
result = matches[1];
}
}

if (!result) {
result = qunitHtmlFile;
}

result = result.replace(/[:*?"<>|]/g, "");
result = result.replace(/[\\/]/g, ".");
result = result.replace(/\.qunit\.html/g, "");
result = result.replace(/\.qunit\./g, "--");
result = result.replace(/\.qunit--/g, "--");

return result;
}

(function(window) {
const karma = window.__karma__;
const exportFiles = [];

function reportSetupFailure(description, error) {
karma.info({total: 1});
Expand Down Expand Up @@ -166,6 +213,14 @@ require("./discovery.js");
// Merge test page coverage into global coverage object
if (!accessError) {
mergeCoverage(testWindow.contentWindow.__coverage__);

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this format is taken from the java implementation

exportFiles.push(file);
});
}
}

// Run next test or trigger completion
Expand All @@ -177,7 +232,8 @@ require("./discovery.js");
// Also merge coverage results from karma window
mergeCoverage(window.__coverage__);
karma.complete({
coverage: coverageMap ? coverageMap.toJSON() : undefined
coverage: coverageMap ? coverageMap.toJSON() : undefined,
exportFiles: config.fileExport ? exportFiles : undefined
});
}
}
Expand Down
13 changes: 12 additions & 1 deletion lib/client/sap-ui-config.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
(function(window) {
const config = window.__karma__.config;
const karma = window.__karma__;
const config = karma.config;
const ui5config = (config && config.ui5) || {};
const bootstrapConfig = ui5config.config || {};

window["sap-ui-config"] = bootstrapConfig;

if (ui5config.fileExport) {
const originalKarmaComplete = karma.complete.bind(karma);
karma.complete = function(result) {
if (window._$files) {
result.exportFiles = window._$files;
}
return originalKarmaComplete(result);
};
}
})(window);
29 changes: 29 additions & 0 deletions lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,35 @@ module.exports = function(config) {
});
};`,

invalidFileExportReporterUsage: () => `error 21:
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

The reporter "ui5--fileExport" should not be manually enabled as a karma reporter.
You can enable the FileExportReporter in the karma-ui5 settings.

module.exports = function(config) {
config.set({

ui5: {
fileExport: true
}

});
};

Optionally, an output directory can be set to specify the export path.

module.exports = function(config) {
config.set({

ui5: {
fileExport: {
outputDir: "directory/to/export/files"
}
}

});
};
`,

failure: () => "ui5.framework failed. See error message above"

}
Expand Down
94 changes: 94 additions & 0 deletions lib/fileExportReporter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
const fs = require("fs").promises;
const path = require("path");
const mkdirp = require("mkdirp");

const defaultPath = "./karma-ui5-reports";

async function getUniqueFileName(fileName) {
async function fileExists(path) {
try {
await fs.access(path);
return true;
} catch (err) {
if (err.code === "ENOENT") {
return false;
}
throw err;
}
}

const fileExtension = path.extname(fileName);
const fileNameWithoutExtension = fileName.slice(0, fileName.lastIndexOf(fileExtension));
Copy link
Member

Choose a reason for hiding this comment

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

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 🤷

Copy link
Member

Choose a reason for hiding this comment

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

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

for (let index = 1; await fileExists(fileName); index++) {
fileName = `${fileNameWithoutExtension}_${index}${fileExtension}`;
}

return fileName;
}

const FileExportReporter = function(baseReporterDecorator, config, logger, helper) {
let reporterInProcess = true;
let reporterCompleted = function() {};
const log = logger.create("reporter.ui5--fileExport");
const reporterConfig = config.ui5.fileExport;
const multiBrowsers = config.browsers && config.browsers.length > 1;
let outputDir = reporterConfig.outputDir;

if (!outputDir || typeof outputDir !== "string") {
outputDir = defaultPath;
}

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

Choose a reason for hiding this comment

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

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));

Copy link
Member

Choose a reason for hiding this comment

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

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


log.debug("outputDir is: " + outputDir);

baseReporterDecorator(this);

async function writeSingleFile(outputFile, content) {
log.info(`Writing file: ${outputFile}`);
Copy link
Member

Choose a reason for hiding this comment

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

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".

Copy link
Member

Choose a reason for hiding this comment

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

Done

await mkdirp(path.dirname(outputFile));
outputFile = await getUniqueFileName(outputFile);
try {
await fs.writeFile(outputFile, content);
log.info(`Saved file '${outputFile}'`);
} catch (err) {
log.warn("Failed to write file\n\t" + err.message);
}
}

this.onBrowserComplete = async function(browser, result) {
log.debug("onBrowserComplete triggered.");
if (!result || result.error || result.disconnected) {
log.debug("skipped due to incomplete test run.");
return;
}

if (!result.exportFiles) {
log.debug("No export files provided");
return;
}

for (const file of result.exportFiles) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Done

const pathSegments = [outputDir, file.name];
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Done

if (multiBrowsers) {
pathSegments.splice(1, 0, browser.name);
Copy link
Member

Choose a reason for hiding this comment

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

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

Same for browser name I guess

Copy link
Member

Choose a reason for hiding this comment

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

Done

}

await writeSingleFile(path.join.apply(null, pathSegments), file.content);
}
reporterInProcess = false;
reporterCompleted();
};

this.onExit = function(done) {
if (reporterInProcess) {
reporterCompleted = done;
} else {
done();
}
};
};

FileExportReporter.$inject = ["baseReporterDecorator", "config", "logger", "helper"];

module.exports = FileExportReporter;
14 changes: 14 additions & 0 deletions lib/framework.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class Framework {
this.config.middleware = config.middleware || [];
this.config.files = config.files || [];
this.config.beforeMiddleware = config.beforeMiddleware || [];
this.config.reporters = this.config.reporters || [];

if (!this.config.ui5.mode) {
this.config.ui5.mode = "html";
Expand Down Expand Up @@ -195,6 +196,17 @@ class Framework {
throw new Error(ErrorMessage.failure());
}

if (this.config.reporters.includes("ui5--fileExport")) {
this.logger.log("error", ErrorMessage.invalidFileExportReporterUsage());
throw new Error(ErrorMessage.failure());
}
if (this.config.ui5.fileExport === true || typeof this.config.ui5.fileExport === "object") {
this.config.reporters.push("ui5--fileExport");
if (this.config.ui5.fileExport === true) {
this.config.ui5.fileExport = {};
}
}

this.config.ui5.paths = this.config.ui5.paths || {
webapp: "webapp",
src: "src",
Expand Down Expand Up @@ -247,6 +259,8 @@ class Framework {
this.config.client.ui5.failOnEmptyTestPage = this.config.ui5.failOnEmptyTestPage;
// Pass configured urlParameters to client
this.config.client.ui5.urlParameters = this.config.ui5.urlParameters;
// Pass fileExport parameter to client
this.config.client.ui5.fileExport = this.config.reporters.includes("ui5--fileExport");


if (this.config.ui5.type === "application") {
Expand Down
4 changes: 3 additions & 1 deletion lib/index.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const {ErrorMessage} = require("./errors");
const Framework = require("./framework");
const FileExportReporter = require("./fileExportReporter");

async function init(config, logger) {
try {
Expand All @@ -24,5 +25,6 @@ getBeforeMiddleware.$inject = getMiddleware.$inject = ["config.ui5"];
module.exports = {
"framework:ui5": ["factory", init],
"middleware:ui5--beforeMiddleware": ["factory", getBeforeMiddleware],
"middleware:ui5--middleware": ["factory", getMiddleware]
"middleware:ui5--middleware": ["factory", getMiddleware],
"reporter:ui5--fileExport": ["type", FileExportReporter]
};
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@
"@ui5/server": "^2.3.1",
"express": "^4.17.1",
"http-proxy": "^1.18.1",
"js-yaml": "^4.1.0"
"js-yaml": "^4.1.0",
"mkdirp": "^1.0.4"
},
"devDependencies": {
"@babel/core": "^7.15.5",
Expand Down
Loading