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

Fixed so that timing will always use native Date for rare cases #240

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Conversation

strille
Copy link
Contributor

@strille strille commented Dec 20, 2019

Since the user might mock Dates in tests, KarmaReporter saves a reference to the native Date and uses that reference for measuring duration. The problem is that it only works when users follows best practises when writing tests.

If a user writes a "bad" test where the Date is mocked in the body of a decribe instead of inside an it or beforeEach, the Date is already mocked by the time KarmaReporter is called.

By saving the reference at the time adapter.js is executed instead of when KarmaReporter is called, this problem is prevented.

Nothing prevents a developer from writing:

describe('foo', function () {
    jasmine.clock().mockDate(new Date(0));
    it('bar', function () {
    });
}

Before the change in this PR, a test like the one above would cause the duration of the test to be reported incorrectly. If the time is reported as negative, then all sorts of thouble can happen down the line.

We ran into issues where a report file generated by karma-sonarqube-unit-reporter containing 4500+ tests was ignored by Sonarqube because a duration in the xml was negative.

…mock even for badly designed tests

The user might mock the Date. Because of this KarmaReporter saved a reference to the native Date and used that reference for measuring duration. The problem was that it only worked when users followed best practises when writing tests. 
If a user wrote a "bad" test where the Date is mocked immediately in the body of a decribe instead of inside an it or beforeEach, the Date was already mocked by the time KarmaReporter was called. By saving the reference at the time adapter.js is executed instead of when KarmaReporter is run, this problem is circumvented.
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@strille
Copy link
Contributor Author

strille commented Dec 20, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@johnjbarton johnjbarton merged commit 28aa72f into karma-runner:master Jan 3, 2020
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

3 participants