Skip to content

Conversation

mikaelm12
Copy link
Contributor

@mikaelm12 mikaelm12 commented Nov 28, 2018

Moving this pr to the release/2.1
Issue: https://github.com/aspnet/AspNetCore-Internal/issues/1320

@analogrelay
Copy link
Contributor

I can't really tell from the output if the AzDO build is passing, but as long as the WebSockets tests are running successfully with this change, it looks good to me.

analogrelay
analogrelay previously approved these changes Nov 29, 2018
Copy link
Contributor

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Approved? If it's working :)

@analogrelay analogrelay dismissed their stale review November 29, 2018 21:45

Looks like it's not running

@analogrelay
Copy link
Contributor

 Skipped  Microsoft.AspNetCore.WebSockets.ConformanceTest.AutobahnTests.AutobahnTestSuite

in the build log.

Maybe the test was marked skipped to prevent further failures?

@ryanbrandenburg
Copy link
Contributor

Likely the problem is this. If it's relatively fast to add wstest installation to the pr-validation build do so, if not I would say to just merge it and keep an eye out for any fallout since this fails 100% of TeamCity tests. Either way make sure there's an (open) issue to remove that constitutionality when there's no place that actively uses it.

@analogrelay
Copy link
Contributor

analogrelay commented Nov 30, 2018

Likely the problem is this. If it's relatively fast to add wstest installation to the pr-validation build do so, if not I would say to just merge it and keep an eye out for any fallout since this fails 100% of TeamCity tests.

We need to know for sure if it's actually going to fix the tests. If @mikaelm12 has successfully run the tests locally, that's sufficient for me.

On a parallel note, we really should enable the tests in the PR validation ASAP. It's pretty simple to set up wstest, the logic is here: https://github.com/aspnet/AspNetCore/blob/master/src/Middleware/WebSockets/setup-wstest.ps1 (and a similar .sh). Basically the steps are:

  1. Install Python (could be done with an AzDO task probably?)
  2. Use Python's package manager pip to install the autobahntestsuite package
  3. Verify that the wstest command is properly on the PATH

The script I linked uses virtualenv because it was designed to be run on an agent and kept around permanently, so it tries to make it easy to remove and isolate from other python installs/builds. If we install it as part of the build steps, I feel much less inclined to bother with virtualenv.

@natemcmaster
Copy link
Contributor

On a parallel note, we really should enable the tests in the PR validation ASAP.

+1. Feel free to open issues on this repo and label as area-infrastructure to track this kind of work.

@mikaelm12
Copy link
Contributor Author

Took a little bit to get the Autobahn test suite set up locally but looks like the tests are passing now
autobahntestpass

@mikaelm12 mikaelm12 merged commit d9a953d into release/2.1 Dec 1, 2018
@natemcmaster natemcmaster deleted the mikaelm12/AutobahnTestFix branch January 18, 2019 19:18
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.

4 participants