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

Add test cases for perf-x database #1597

Merged
merged 3 commits into from
Feb 2, 2017

Conversation

weiwei-lin
Copy link
Contributor

Added some test cases for perf-x database
Also refactored the database a little bit and fixed a bug.

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

how about an assertion that clear() successfully clears?

describe('Perf-X Database', function() {
it('can store and output experiments data', () => {
const perfXDatabase = new PerfXDatabase();
process.on('exit', () => perfXDatabase.clear());
Copy link
Member

Choose a reason for hiding this comment

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

i'd put this in an afterEach block (some other tests have them)

Copy link
Member

Choose a reason for hiding this comment

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

and do you want to use a fresh database for each of these test cases? if so you could do setup/teardown in beforeEach/afterEach inside the describe and do each test case in their own it.

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'd put this in an afterEach block (some other tests have them)

I didn't know that! Thx for your advice

and do you want to use a fresh database for each of these test cases?

Are you referring to testCases or 'it'? I want to test the whether database is capable of storing multiple data sets here. I will rename testCases to dataSets to make it clear.

},
{
flags: {
'blockedUrlPatterns': ['.woff', 'cat.jpg', '*'],
Copy link
Member

Choose a reason for hiding this comment

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

pretty odd we have this mix of camel casing and not in the flags. Why arent the disable flags in camelcase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const flagsBeforeChange = JSON.parse(JSON.stringify(perfXDatabase.getFlags(key)));
const resultsBeforeChange = JSON.parse(JSON.stringify(perfXDatabase.getResults(key)));

// data won't be changed when the falgs/results passed to perfXDatabase.saveData is changed
Copy link
Member

Choose a reason for hiding this comment

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

falgs => flags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.


testCases.forEach(testCase => {
assert.strictEqual(perfXDatabase.timeStamps[testCase.key], testCase.results.generatedTime);
});
Copy link
Member

Choose a reason for hiding this comment

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

how about an assertion about perfXDatabase.timeStamps.length? (3 is expected)

(I presume this would fail right now, until the process.on exit is changed to an afterEach..)

Copy link
Contributor Author

@weiwei-lin weiwei-lin Feb 1, 2017

Choose a reason for hiding this comment

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

how about an assertion about perfXDatabase.timeStamps.length? (3 is expected)

Thx for your advice. I will add that.

@paulirish paulirish merged commit 575447a into GoogleChrome:master Feb 2, 2017
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.

None yet

2 participants