Skip to content

Fix SignalR typescript tests with Chrome SameSite reaction #25283

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
Merged
merged 2 commits into from
Aug 26, 2020
Merged

Fix SignalR typescript tests with Chrome SameSite reaction #25283

merged 2 commits into from
Aug 26, 2020

Conversation

@BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Aug 26, 2020

Chrome released its SameSite cookie support on 8/25 which broke some tests that used cookies cross-domain. This fixes the tests.

@BrennanConroy BrennanConroy added area-signalr Includes: SignalR clients and servers tell-mode Indicates a PR which is being merged during tell-mode labels Aug 26, 2020
@BrennanConroy BrennanConroy added this to the 5.0.0-rc1 milestone Aug 26, 2020
@BrennanConroy BrennanConroy requested a review from Pilchie August 26, 2020 20:15
@Pilchie
Copy link
Member

Pilchie commented Aug 26, 2020

Thanks @BrennanConroy - FYI test-only fixes can be proactively merged unless you have concern about the risk to the build.

@Pilchie Pilchie added the Servicing-approved Shiproom has approved the issue label Aug 26, 2020
@BrennanConroy
Copy link
Member Author

BrennanConroy commented Aug 26, 2020

proactively merged unless you have concern about the risk to the build

Mac passed on this PR which was the remaining failures on #25090 which had half of this change. I've verified before and after on my Windows machine as well, so confident this wont break.

If someone wants they can merge then 👍

@dougbu dougbu merged commit f2b72b0 into release/5.0 Aug 26, 2020
@dougbu dougbu deleted the brecon/samesite branch August 26, 2020 21:23
@dougbu
Copy link
Contributor

dougbu commented Aug 26, 2020

@ajaybhargavb the validation build for this PR failed due to build ordering issues. Something has destabilized the release/5.0 branch significantly because I've seen this a few times in the last week. Please use some of your ops-on-call time to figure out what's busted.

@ajaybhargavb
Copy link
Contributor

ajaybhargavb commented Aug 26, 2020

@dougbu, I'm currently heads down on getting #25219 merged. Would you like me to file an issue to track this?

@dougbu
Copy link
Contributor

dougbu commented Aug 26, 2020

@ajaybhargavb yes please. And, please also mention that issue as important when handing off your ops rotation.

@ajaybhargavb ajaybhargavb mentioned this pull request Aug 26, 2020
Closed
@ajaybhargavb
Copy link
Contributor

ajaybhargavb commented Aug 26, 2020

Filed #25293

BrennanConroy added a commit that referenced this pull request Aug 31, 2020
@BrennanConroy
Fix SignalR typescript tests with Chrome SameSite reaction (#25283)
fa8eb31 
* Fix Typescript tests
* fixup
@BrennanConroy BrennanConroy mentioned this pull request Aug 31, 2020
Closed
@dougbu
Copy link
Contributor

dougbu commented Aug 31, 2020

@BrennanConroy thanks for opening #25473 too❕

You'll probably need to cherry-pick @captainsafia's #25324 and skip some of the Kestrel Interop tests to get that PR working. When you succeed, I'll copy what you've done to test out my #25217😺

BrennanConroy added a commit that referenced this pull request Sep 1, 2020
@BrennanConroy
Fix SignalR typescript tests with Chrome SameSite reaction (#25283)
e7cb2dd
wtgodbe pushed a commit that referenced this pull request Sep 8, 2020
@captainsafia @BrennanConroy @dougbu
Set SameSiteMode for cookies in authentication tests (#25281) (#25324)
bacee80 
* Set SameSiteMode for cookies in authentication tests (#25281)

* Fix SignalR typescript tests with Chrome SameSite reaction (#25283)

* Disable template tests that use Selenium

Co-authored-by: Brennan <[email protected]>
Co-authored-by: Doug Bunting <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tratcher Tratcher Tratcher approved these changes

@wtgodbe wtgodbe wtgodbe approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@dougbu dougbu Awaiting requested review from dougbu

@ajaybhargavb ajaybhargavb Awaiting requested review from ajaybhargavb

@Pilchie Pilchie Awaiting requested review from Pilchie

Assignees

No one assigned

Labels

area-signalr Includes: SignalR clients and servers Servicing-approved Shiproom has approved the issue tell-mode Indicates a PR which is being merged during tell-mode

Projects

None yet

Milestone

5.0.0-rc1

Development

Successfully merging this pull request may close these issues.

7 participants

@BrennanConroy @Pilchie @dougbu @ajaybhargavb @Tratcher @wtgodbe