Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

Move from travis to github actions #55

Merged
merged 6 commits into from
Aug 31, 2021
Merged

Conversation

natebosch
Copy link
Contributor

@natebosch natebosch requested a review from kevmoo August 30, 2021 23:01
Copy link
Contributor

@grouma grouma left a comment

Choose a reason for hiding this comment

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

Looks like there are some lint issues but otherwise LGTM.

@kevmoo
Copy link
Contributor

kevmoo commented Aug 31, 2021

Grr...one failing test!

…same connection is sufficient for the intent of the test

// Ensure there's still a connection and it's marked as in the keep-alive
// state.
expect(connection.isInKeepAlivePeriod, isTrue);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grouma - Do you think that the round tripping of the value 'bar' through the same connection object as was used before and already closed should be sufficient for this test?

I could add a stream of reconnect events, but I don't think it's necessary to demonstrate correctness here.
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's probably fine since closeSink is synchronous and should guarantee that the connection is severed before we send bar.

@natebosch natebosch merged commit 9084339 into master Aug 31, 2021
@natebosch natebosch deleted the move-to-github-actions branch August 31, 2021 22:38
mosuem pushed a commit to dart-lang/tools that referenced this pull request Dec 10, 2024
Closes dart-lang/sse#54

Fix formatting in one file.

Remove test expectations around keep alive periods.
Testing communication through the same connection that
had been closed is sufficient for the intent of the test.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Migrate to github actions
3 participants