thaliproject / Thali_CordovaPlugin

Thali p2p plugin
MIT License
226 stars 44 forks source link

Is WiFi only mode in tests actually testing WiFi Only mode? #1167

Closed yaronyg closed 7 years ago

yaronyg commented 7 years ago

@artemjackson - although this bug belongs to Andrew I'd appreciate it if you would look at it as well just to give commentary since you did the original work here and thus have context I lack.

@larryonoff / @enricogior - You two will need to pay close attention to this one because if I'm right then our wifi only tests haven't been testing wifi. They have also used native layer. Although as I explain below that would seem kinda odd so I'm hoping I'm missing something.

To control the mode that tests run in we are supposed to set a variable called networkTypes in UnitTest_App.js. This is then used to set a variable global.NETWORK_TYPE which the tests are supposed to examine while running.

But right now global.NETWORK_TYPE is only checked in exactly three places, testThaliMobileNative, testThaliMobileNativeWrapper and testThaliWifiInfrastructure. Which means all the other test files are ignoring it, right? Specifically, thaliMobile takes an argument to start that sets its network type. If not provided that argument defaults to 'both'.

So that means that all the testing we are doing for higher level functionality aren't honoring NETWORK_TYPE and are trying to use both? The reason for the question mark is that if this were the case then I would assume that @evabishchevich would have seen native traffic in the logs when he did WiFi only testing. I also would have expected that @dersim-davaod and @baydet would have seen all sorts of issues when they ran WiFi only tests in iOS.

So I'm confused. If I'm reading the code right, we aren't actually testing in WiFi only mode any of the higher level stuff at all! So am I confused?

If I'm not confused then we need to fix this asap. The fix I would propose is:

artemjackson commented 7 years ago

@yaronyg Unfortunatly, but you are right. All of 5 items of the list below can be fixed ASAP by adding a check like

thaliMobileStates.networkType = 
  networkType || global.NETWORK_TYPE || thaliMobileStates.networkType

This will allow not to force users to pass networkType into ThaliMobile.start method. Furthermore it solves issue with pskIdToSecret more elegantly and there is no need to fix all the test that calls ThaliMobile.start (because it will use global network type) And btw #1009 seems to be redundant with this approach.

I've already implemented it here #1176.