Skip to content

Move shared tests to shared#837

Closed
GantMan wants to merge 13 commits intomicrosoft:masterfrom
infinitered:move_tests
Closed

Move shared tests to shared#837
GantMan wants to merge 13 commits intomicrosoft:masterfrom
infinitered:move_tests

Conversation

@GantMan
Copy link
Copy Markdown
Contributor

@GantMan GantMan commented Oct 28, 2016

Shared tests migrated to shared, to help test Net 4.6

@rozele
Copy link
Copy Markdown
Contributor

rozele commented Oct 28, 2016

I think @matthargett mentioned there would either be NMock support for UWP or a decision to remove it throughout by EOW (which is EOD today). Can we wait on migrating further tests until that is resolved?

@rozele
Copy link
Copy Markdown
Contributor

rozele commented Oct 28, 2016

None of these tests use NMock, but I don't want to sign off on removing yet more test coverage from UWP


In reply to: 257005127 [](ancestors = 257005127)

@GantMan
Copy link
Copy Markdown
Contributor Author

GantMan commented Oct 28, 2016

Yup! Sorry, this existed, and wanted to put it in a stable place for a hand-off if it goes forward.

@GantMan
Copy link
Copy Markdown
Contributor Author

GantMan commented Oct 28, 2016

@rozele - Sounds like there is a plan from @matthargett - so this PR is just for reference as they implement separate testing.

@matthargett
Copy link
Copy Markdown
Contributor

yes. @pre10der89 submitted PR #840 , which moves the few NMock-based tests to the ReactNative.Net46.Tests project, and adds ReactNative.Tests.Shared reference to the ReactNative.Tests Universal project. Once #840 is merged, this PR can be merged at no cost to coverage of the ReactNative Universal project.

@kevinvangelder
Copy link
Copy Markdown
Contributor

I ran into trouble migrating Bridge/ReactInstanceTests.cs and Modules/Storage/AsyncStorageModuleTests.cs (they ran forever) so I'm not including them in this PR.

@kevinvangelder
Copy link
Copy Markdown
Contributor

So, I guess I can't git... In the previous commit I moved the tests from UWP to shared and then tweaked them to be shared, but I guess that because the end result was closer to the Net46 tests (which I deleted) GitHub has decided to be "helpful" and change it so that I "moved" the Net46 tests and "deleted" the UWP tests...

WAT. THE. WAT.

@kevinvangelder
Copy link
Copy Markdown
Contributor

using System.Net;
using System.Net.Http;
using HttpStreamContent = System.Net.Http.StreamContent;
#endif
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.

I don't think this is needed, we were only using it when HttpStreamContent was an output of a method. Here, you're just making constructor calls isolated in #ifs anyway, so its better not to type alias.

Copy link
Copy Markdown
Contributor

@rozele rozele left a comment

Choose a reason for hiding this comment

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

Just the one comment about the type alias in NetworkingModuleTests then good to go.

kevinvangelder added a commit to infinitered/react-native-windows that referenced this pull request Nov 18, 2016
kevinvangelder added a commit to infinitered/react-native-windows that referenced this pull request Nov 22, 2016
@kevinvangelder
Copy link
Copy Markdown
Contributor

Replaced by #889 & #899

@rozele
Copy link
Copy Markdown
Contributor

rozele commented Nov 28, 2016

@kevinvangelder can we close this if its superceded by #889 and #899?

@kevinvangelder
Copy link
Copy Markdown
Contributor

@rozele Yes please, I didn't open the PR so I couldn't close it and hadn't gotten around to asking @GantMan to close it yet.

@GantMan GantMan closed this Nov 29, 2016
rozele pushed a commit that referenced this pull request Dec 9, 2016
* Re-recreate #837 but moving from UWP w/o submodule changes

* disabled appveyor cache settings to see if it fixes our test discovery issue

* testing appveyor

* hopefully fixed AppVeyor build

* maybe separating the commands will help?

* separate commands for real

* hopefully working appveyor build

* added missing $

* fixing error in appveyor.yml?

* fixed appveyor for real this time
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