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

chore(ci): re-enable tests on Windows #5637

Merged
merged 5 commits into from Apr 17, 2020
Merged

chore(ci): re-enable tests on Windows #5637

merged 5 commits into from Apr 17, 2020

Conversation

jackfranklin
Copy link
Collaborator

@jackfranklin jackfranklin commented Apr 14, 2020

No description provided.

@jackfranklin jackfranklin changed the title [WIP] Windows on CI Add Windows tests to Travis CI Apr 14, 2020
@AviVahl

This comment has been minimized.

test/network.spec.js Outdated Show resolved Hide resolved
test/network.spec.js Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
@jackfranklin jackfranklin force-pushed the travis-windows branch 2 times, most recently from c61b5ce to ceab809 Compare April 17, 2020 12:41
This commit runs the unit tests on Windows.

There are two tests failing on Windows that we skip.

I spoke to Mathias B and we agreed to
defer debugging this for now in favour of getting tests running on
Windows. But we didn't want to ignore it forever, hence giving the test
a date at which it will start to fail.
Copy link
Member

@mathiasbynens mathiasbynens left a comment

Choose a reason for hiding this comment

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

LGTM with some final nitpicks

test/headful.spec.js Outdated Show resolved Hide resolved
test/launcher.spec.js Outdated Show resolved Hide resolved
@@ -92,6 +93,16 @@ global.itFailsFirefox = (...args) => {
return it(...args);
};

global.itFailsWindowsUntilDate = (date, ...args) => {
const today = new Date();
Copy link
Member

Choose a reason for hiding this comment

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

Since we only need the timestamp to compare:

Suggested change
const today = new Date();
const today = Date.now;

Since this is pretty short we don't really need the today variable (better: now?) anymore.

Copy link
Member

Choose a reason for hiding this comment

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

We could make this function accept a string for the date parameter, since all callers do new Date(string) anyway.

Copy link
Member

Choose a reason for hiding this comment

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

i.e. itFailsWindowsUntilDate('2020-06-01', blah)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

May do that in a follow up PR - I'm also tempted to tidy up the test helpers a bit. Right now we have itFailsFirefox (with no date) and itFailsWindowsUntilDate. I'd quite like a itFails({ product: 'firefox', date: '... }) or similar API rather than bespoke methods every time

test/network.spec.js Outdated Show resolved Hide resolved
jackfranklin and others added 4 commits April 17, 2020 13:53
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Co-Authored-By: Mathias Bynens <mathias@qiwi.be>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants