Skip to content

fix(tests): minor fix for transportMode.client test case - #2183

Closed
jamesgeorge007 wants to merge 1 commit into
webpack:masterfrom
jamesgeorge007:hotfix/fix-transportmode-client-test
Closed

fix(tests): minor fix for transportMode.client test case#2183
jamesgeorge007 wants to merge 1 commit into
webpack:masterfrom
jamesgeorge007:hotfix/fix-transportmode-client-test

Conversation

@jamesgeorge007

@jamesgeorge007jamesgeorge007 commented Aug 9, 2019

Copy link
Copy Markdown
Contributor
  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

N/A

Motivation / Use-Case

There was a slight typo in #2116 that got unnoticed.

Breaking Changes

N/A

Additional Info

N/A

@codecov

codecovBot commented Aug 9, 2019

Copy link
Copy Markdown

Codecov Report

Merging #2183 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@## master #2183 +/- ##
=======================================
Coverage 96.08% 96.08% =======================================
Files 34 34 Lines 1227 1227 Branches 349 349 =======================================
Hits 1179 1179 Misses 47 47 Partials 1 1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5b9cb4...63c2e12. Read the comment docs.

@hiroppyhiroppy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, we accept a string.

@hiroppy

Copy link
Copy Markdown
Contributor

@hiroppy

hiroppy commented Aug 9, 2019

Copy link
Copy Markdown
Contributor

Why do you introduce this change?

We must add these cases.

string: transportMode: 'sockjs', 'ws'
object: transportMode.server, transportMode.client

And we've already had enough test cases.

This change breaks the above test cases.

@jamesgeorge007

Copy link
Copy Markdown
ContributorAuthor

I assumed this from the diff (#2116) such that the change was specific to clientMode (transportMode.client)

@jamesgeorge007

Copy link
Copy Markdown
ContributorAuthor

As seen from the test-case it is something specific to transportMode client:-
https://github.com/Loonride/webpack-dev-server/blob/aa17f7ca58eb3bd97a097f4901b58295acbe7334/test/e2e/TransportMode.test.js#L12-18

@alexander-akait

alexander-akait commented Aug 9, 2019

Copy link
Copy Markdown
Member

@jamesblight what is problem here?

We support transportMode: 'ws and transportMode: { client: 'ws' , 'server': 'ws'}?

@jamesgeorge007

jamesgeorge007 commented Aug 9, 2019

Copy link
Copy Markdown
ContributorAuthor

@evilebottnawi as per what I see, the test was specific for transportMode clienthere
As evident from the diff, the respective test case was specific to options.clientMode (before #2116) which should be set to transportMode.client right 🤔
Screenshot from 2019-08-09 15-47-41
Instead, the entire transportMode object was being set.

@jamesgeorge007

Copy link
Copy Markdown
ContributorAuthor

Hope I was able to convey the idea 🤔

@jamesgeorge007

Copy link
Copy Markdown
ContributorAuthor

@jamesblight what is problem here?

wrong user mentioned 😅

@alexander-akait

Copy link
Copy Markdown
Member

I think test is right here

@jamesgeorge007

jamesgeorge007 commented Aug 9, 2019

Copy link
Copy Markdown
ContributorAuthor

@evilebottnawi@hiroppy

options: {clientMode: 'sockjs'}

was updated to be:-

options: {transportMode: 'sockjs'}

As per what I infer, instead of having clientMode and serverMode#2116 introduced transportMode as:-

transportMode: {client: '',server: ''}

Hence, options.transportMode.client should be an equivalent of options.clientMode right 🤔
Instead the respective case made use of just options.transportMode which made me confused 😕

@alexander-akait

Copy link
Copy Markdown
Member

options.clientMode and options.serverMode were removed, now only transportMode option

@jamesgeorge007

Copy link
Copy Markdown
ContributorAuthor

Even transportMode comprises of client and server entries right?

@alexander-akait

Copy link
Copy Markdown
Member

@jamesgeorge007 yes, you can set transportMode: { client: myImplementation, server: 'ws} for example

@jamesgeorge007
jamesgeorge007 deleted the hotfix/fix-transportmode-client-test branch August 9, 2019 11:00
@knagaitsev

Copy link
Copy Markdown
Contributor

@jamesgeorge007

When we just had:

options: {
clientMode: 'sockjs'
}

We were relying on the fact that the default serverMode was also sockjs, so that server and client could communicate. This is still the case, but in the future, the default mode will be changed to ws.

This is better, because it specifies both the client and server modes explicitly:

options: {
transportMode: 'sockjs'
}

since it is equivalent to:

options: {
transportMode: {
client: 'sockjs',
server: 'sockjs'
}
}

Sign up for freeto 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

@jamesgeorge007@hiroppy@alexander-akait@knagaitsev