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.

Sign up for GitHub

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

Jump to bottom

Convert to classes and add suitenames attributes #61

Merged
merged 17 commits into from
Feb 1, 2024
Merged

Convert to classes and add suitenames attributes #61

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
9f3b9c8
refactor: convert to class syntax
SimeonC Jan 19, 2024
af8bcaf
feat: refactor to support testsuites root element attributes/properties
SimeonC Jan 19, 2024
62793ca
chore: Remove code comment
davidparsson Feb 1, 2024
ea27a99
refactor: Remove unused default values
davidparsson Feb 1, 2024
ac308fa
refactor: Move child element list to `TestGroup`
davidparsson Feb 1, 2024
5ebe11d
Move `className()` back to `TestCase`
davidparsson Feb 1, 2024
c405574
refactor: Rename parameter for consistency
davidparsson Feb 1, 2024
19ac2bf
refactor: Rename method
davidparsson Feb 1, 2024
934d355
refactor: Use the factory
davidparsson Feb 1, 2024
d13908a
style: Move important public method to the top
davidparsson Feb 1, 2024
ada9e92
Set the name of `<testsuites>` from the builder
davidparsson Feb 1, 2024
2161361
style: Use a real function body for the test
davidparsson Feb 1, 2024
a84446b
refactor: More XML in e2e tests
davidparsson Feb 1, 2024
2be8b1b
refactor: Create root element in `TestNode`
davidparsson Feb 1, 2024
ebd06f4
style: Public methods at the top
davidparsson Feb 1, 2024
45e59f1
docs: Add missing return type
davidparsson Feb 1, 2024
848a18f
Update changelog for the upcoming release
davidparsson Feb 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 50 additions & 3 deletions spec/e2e_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,28 @@ describe('JUnit Report builder', function () {
}),
);

const reportWith = (content) =>
'<?xml version="1.0" encoding="UTF-8"?>\n' + '<testsuites>\n' + content + '\n' + '</testsuites>';
const parseProperties = (properties = {}) => {
let result = '';
['tests', 'failures', 'errors', 'skipped'].forEach((key) => {
result += key + '="' + (properties[key] || 0) + '" ';
});
for (const key in properties) {
if (['tests', 'failures', 'errors', 'skipped'].includes(key)) {
continue;
}
result += key + '="' + properties[key] + '" ';
}
return result.trim();
};
davidparsson marked this conversation as resolved.
Show resolved Hide resolved

const reportWith = (content, testSuitesProperties) =>
'<?xml version="1.0" encoding="UTF-8"?>\n' +
'<testsuites ' +
parseProperties(testSuitesProperties) +
'>\n' +
content +
'\n' +
'</testsuites>';

it('should produce a report identical to the expected one', function () {
builder.testCase().className('root.test.Class1');
Expand Down Expand Up @@ -44,16 +64,26 @@ describe('JUnit Report builder', function () {
expect(builder.build()).toBe(
// prettier-ignore
'<?xml version="1.0" encoding="UTF-8"?>\n' +
'<testsuites/>',
'<testsuites tests="0" failures="0" errors="0" skipped="0"/>',
));

it('should set testsuites name', () => {
builder.testSuites().name('testSuitesName');
expect(builder.build()).toBe(
// prettier-ignore
'<?xml version="1.0" encoding="UTF-8"?>\n' +
'<testsuites name="testSuitesName" tests="0" failures="0" errors="0" skipped="0"/>',
);
});

it('should produce an empty test suite when a test suite reported', function () {
builder.testSuite();

expect(builder.build()).toBe(
reportWith(
// prettier-ignore
' <testsuite tests="0" failures="0" errors="0" skipped="0"/>',
{ tests: 0, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -65,6 +95,7 @@ describe('JUnit Report builder', function () {
reportWith(
// prettier-ignore
' <testcase/>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -78,6 +109,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <failure message="it failed"/>\n' +
' </testcase>',
{ tests: 1, failures: 1, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -91,6 +123,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <failure message="it failed" type="the type"/>\n' +
' </testcase>',
{ tests: 1, failures: 1, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -104,6 +137,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <error message="it errored"/>\n' +
' </testcase>',
{ tests: 1, failures: 0, errors: 1, skipped: 0 },
),
);
});
Expand All @@ -117,6 +151,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <error message="it errored" type="the type"><![CDATA[the content]]></error>\n' +
' </testcase>',
{ tests: 1, failures: 0, errors: 1, skipped: 0 },
),
);
});
Expand All @@ -130,6 +165,7 @@ describe('JUnit Report builder', function () {
' <testsuite tests="1" failures="0" errors="0" skipped="0">\n' +
' <testcase/>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -145,6 +181,7 @@ describe('JUnit Report builder', function () {
' <failure/>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 1, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -160,6 +197,7 @@ describe('JUnit Report builder', function () {
' <error/>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 1, skipped: 0 },
),
);
});
Expand All @@ -175,6 +213,7 @@ describe('JUnit Report builder', function () {
' <skipped/>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 1 },
),
);
});
Expand Down Expand Up @@ -210,6 +249,7 @@ describe('JUnit Report builder', function () {
' <testsuite tests="1" failures="0" errors="0" skipped="0">\n' +
' <testcase time="2.5"/>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -225,6 +265,7 @@ describe('JUnit Report builder', function () {
' <system-out><![CDATA[This was written to stdout!]]></system-out>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -240,6 +281,7 @@ describe('JUnit Report builder', function () {
' <system-err><![CDATA[This was written to stderr!]]></system-err>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -262,6 +304,7 @@ describe('JUnit Report builder', function () {
' </system-err>\n' +
' </testcase>\n' +
' </testsuite>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -277,6 +320,7 @@ describe('JUnit Report builder', function () {
' <testcase name="1"/>\n' +
' <testsuite name="2" tests="0" failures="0" errors="0" skipped="0"/>\n' +
' <testcase name="3"/>',
{ tests: 2, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -290,6 +334,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <system-out><![CDATA[Emoji: 🤦]]></system-out>\n' +
' </testcase>',
{ tests: 1, failures: 0, errors: 0, skipped: 0 },
),
);
});
Expand All @@ -303,6 +348,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <error message="it is &quot;quoted&quot;"/>\n' +
' </testcase>',
{ tests: 1, failures: 0, errors: 1, skipped: 0 },
),
);
});
Expand All @@ -316,6 +362,7 @@ describe('JUnit Report builder', function () {
' <testcase>\n' +
' <error message="InvalidCharactersStripped"/>\n' +
' </testcase>',
{ tests: 1, failures: 0, errors: 1, skipped: 0 },
),
);
});
Expand Down
2 changes: 1 addition & 1 deletion spec/expected_report.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
<testsuites tests="6" failures="2" errors="0" skipped="1">
<testcase classname="root.test.Class1"/>
<testsuite name="first.Suite" tests="2" failures="0" errors="0" skipped="0">
<testcase name="Second test"/>
Expand Down
2 changes: 1 addition & 1 deletion spec/test_case_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const TestCase = require('../src/test_case');
const { TestCase } = require('../src/test_case');

describe('Test Case builder', function () {
let testCase = null;
Expand Down
12 changes: 10 additions & 2 deletions spec/test_suite_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const TestSuite = require('../src/test_suite');
const { TestSuite } = require('../src/test_suite');

describe('Test Suite builder', function () {
let testSuite = null;
Expand All @@ -9,7 +9,13 @@ describe('Test Suite builder', function () {

beforeEach(function () {
const factory = jasmine.createSpyObj('factory', ['newTestCase']);
testCase = jasmine.createSpyObj('testCase', ['build', 'getFailureCount', 'getErrorCount', 'getSkippedCount']);
testCase = jasmine.createSpyObj('testCase', [
'build',
'getFailureCount',
'getErrorCount',
'getSkippedCount',
'getTestCaseCount',
]);

factory.newTestCase.and.callFake(() => testCase);

Expand All @@ -33,6 +39,8 @@ describe('Test Suite builder', function () {
}
});

testCase.getTestCaseCount.and.callFake(() => 1);

testCase.getFailureCount.and.callFake(() => 0);

testCase.getErrorCount.and.callFake(() => 0);
Expand Down
41 changes: 19 additions & 22 deletions src/builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
// @ts-check
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to run a type check for all files with @ts-check on CI somehow? It would be neat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can be done with typescript, but at that point may as well convert it all to typescript anyway as I don't think there's limits to the types we can define in JSDOC

var _ = require('lodash');
var xmlBuilder = require('xmlbuilder');
var path = require('path');
var makeDir = require('make-dir');
var fs = require('fs');
var TestSuite = require('./test_suite');
var TestCase = require('./test_case');
var { TestSuites } = require('./test_suites');

class JUnitReportBuilder {
/**
* @param {import('./factory')} factory
* @param {import('./factory').Factory} factory
*/
constructor(factory) {
this._factory = factory;
this._testSuitesAndCases = [];
this._rootTestSuites = new TestSuites(factory);
}

/**
Expand All @@ -25,40 +22,40 @@ class JUnitReportBuilder {
}

/**
* @returns {string} xml file content
* @returns {string}
*/
build() {
var xmlTree = xmlBuilder.create('testsuites', { encoding: 'UTF-8', invalidCharReplacement: '' });
_.forEach(this._testSuitesAndCases, function (suiteOrCase) {
suiteOrCase.build(xmlTree);
});
var xmlTree = this._rootTestSuites.build();
return xmlTree.end({ pretty: true });
}

/**
* @returns {import('./test_suite')}
* @returns {import('./test_suites').TestSuites}
*/
testSuites() {
return this._rootTestSuites;
}

/**
* @returns {import('./test_suite').TestSuite}
*/
testSuite() {
var suite = this._factory.newTestSuite();
this._testSuitesAndCases.push(suite);
return suite;
return this._rootTestSuites.testSuite();
}

/**
* @returns {import('./test_case')}
* @returns {import('./test_case').TestCase}
*/
testCase() {
var testCase = this._factory.newTestCase();
this._testSuitesAndCases.push(testCase);
return testCase;
return this._rootTestSuites.testCase();
}

/**
* @returns {ReturnType<import('./factory')['newBuilder']>}
* @returns {JUnitReportBuilder}
*/
newBuilder() {
return this._factory.newBuilder();
return new JUnitReportBuilder(this._factory);
davidparsson marked this conversation as resolved.
Show resolved Hide resolved
}
}

module.exports = JUnitReportBuilder;
module.exports = { Builder: JUnitReportBuilder };
Copy link
Owner

Choose a reason for hiding this comment

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

Are changes like this one completely backwards-compatible, or do they require adding curly brackets to the import statements (like this var { Builder } = require('./builder');)?

If they break backwards-compatibility, are they necessary? Best practice?

It might maybe be safe to assume that everyone uses (and can continue to use) this as the only import?

var builder = require('junit-report-builder');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside of src/index.js this won't have any affect as require('junit-report-builder') get's the default export of that file, not these files I refactored, because it resolves to the main field in package.json.

Technically they are breaking changes if someone does require('junit-report-builder/src/builder') then it will break, but technically we're not supposed to do that with node_modules 😂.

That said, this is a habit of mine - doing exports like this makes sure that we use the same name everywhere or explicitly rename it.

24 changes: 15 additions & 9 deletions src/factory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,36 @@
var Builder = require('./builder');
var TestSuite = require('./test_suite');
var TestCase = require('./test_case');
var { Builder } = require('./builder');
var { TestSuites } = require('./test_suites');
var { TestSuite } = require('./test_suite');
var { TestCase } = require('./test_case');

class Factory {
constructor() {}

/**
* @returns {Builder}
* @returns {import('./builder').Builder}
*/
newBuilder() {
return new Builder(this);
}

/**
* @returns {TestSuite}
* @returns {import('./test_suite').TestSuite}
*/
newTestSuite() {
return new TestSuite(this);
}

/**
* @returns {TestCase}
* @returns {import('./test_case').TestCase}
*/
newTestCase() {
return new TestCase(this);
}

/**
* @returns {import('./test_suites').TestSuites}
*/
newTestSuites() {
return new TestSuites(this);
}
}

module.exports = Factory;
module.exports = { Factory: Factory };
2 changes: 1 addition & 1 deletion src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
var Factory = require('./factory');
var { Factory } = require('./factory');

module.exports = new Factory().newBuilder();
Loading
Loading