Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Update karma so we can remove Safari workaround #16743

Merged
merged 4 commits into from
Nov 23, 2018

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Oct 25, 2018

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Fix/Feature: Docs have been added/updated
  • Fix/Feature: Tests have been added; existing tests pass

Other information:

@Narretz Narretz added this to the 1.7.x milestone Oct 25, 2018
@Narretz Narretz changed the title Chore karma update Update karma so we can remove Safari workaround Oct 25, 2018
@Narretz Narretz force-pushed the chore-karma-update branch from edabbb0 to fb649c1 Compare October 25, 2018 12:45
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

🎉

@Narretz Narretz force-pushed the chore-karma-update branch 3 times, most recently from 276f55a to 5a3108a Compare October 26, 2018 16:30
@Narretz
Copy link
Contributor Author

Narretz commented Oct 26, 2018

with the update, Safari 10 fails every time ...

@Narretz Narretz force-pushed the chore-karma-update branch from 5a3108a to 1176d17 Compare October 26, 2018 16:50
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

Magnifique!

@Narretz Narretz force-pushed the chore-karma-update branch from 6c5050e to 14fc726 Compare October 29, 2018 10:08
version: 'latest-1'
},
'SL_Safari': {
base: 'SauceLabs',
browserName: 'safari',
platform: 'OS X 10.12',
platform: 'OS X 10.13',
Copy link
Member

Choose a reason for hiding this comment

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

This may start failing soon as Safari 12 might only be available in Mojave (that's how it is in BrowserStack, for example) - i.e. 10.14. Do we have to specify the platform? If the answer's yes, maybe Safari cannot really be used as latest & latest-1 versions...

Copy link
Member

Choose a reason for hiding this comment

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

The same remark applies to iOS BTW.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When it starts to fail, we can update it. But I can also try it without the explicit platform.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

The last commit looks good

@Narretz Narretz force-pushed the chore-karma-update branch from 98650b4 to eb37e2d Compare October 30, 2018 10:36
This allows us to remove the workaround added in angular#16645
This reverts commit 6b915ad.
Karma has this workaround built in since 3.1.0:
karma-runner/karma@873e4f9
Safari 10 does not finish the tests, but Safari 11 and 12 do
They take a lot of time since we created different karma jobs for them
@Narretz Narretz force-pushed the chore-karma-update branch 7 times, most recently from b1294c2 to 349e13f Compare November 9, 2018 12:03
@Narretz Narretz force-pushed the chore-karma-update branch from 349e13f to a376a0f Compare November 9, 2018 14:37
@Narretz Narretz force-pushed the chore-karma-update branch 5 times, most recently from caed010 to 74d9745 Compare November 12, 2018 16:50
@Narretz Narretz merged commit a5a98d3 into angular:master Nov 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants