Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable Shared tests for the Universal build #840

Merged
merged 28 commits into from Nov 2, 2016

Conversation

pre10der89
Copy link
Contributor

@pre10der89 pre10der89 commented Oct 28, 2016

Enable shared tests for universal build.

…... Any TestFixtures that utilize the NMock3 library need to be isolated in the .NET test project(s)...

Moved NMock3 related tests from ReactiveNative.Shared.Tests to ReactNative46.Tests.

The majority of tests in JavaScriptModuleBaseTests.cs utilized NMock so the entire file was moved from the ReactiveNative.Shared.Tests to the ReactNative.Net46.Tests projects.

A handful of tests in JavaScriptModuleRegistryTests.cs utilized NMock. A specialized class inheriting from JavaScriptModuleRegistryTests was added to the ReactNative.Net46.Tests project.  The specialized class "JavaScriptModuleRegistryNMockTests" contains only the NMock dependent tests and the base class contains all tests that can be shared.

All references to NMock have been removed from the test code in the ReactNative.Shared.Tests projects...

A reference to the ReactNative.Shared.Tests has been added to the ReactNative.Tests project.   Additionally, the NUnit v3.41 nuGet package and the NUnit3TestAdapter v 3.5 nuGet package were added to the ReactNative.Tests project.

The NUnit3TestAdapter v 3.41 nuGet package could not be loaded in the ReactNative.Tests project so v3.5 (the latest) was used. This creates a version mismatch with the NUnit package and the nuGet packages used in the ReactNative.Net46.Tests project...
…... Any TestFixtures that utilize the NMock3 library need to be isolated in the .NET test project(s)...

Moved NMock3 related tests from ReactiveNative.Shared.Tests to ReactNative46.Tests.

The majority of tests in JavaScriptModuleBaseTests.cs utilized NMock so the entire file was moved from the ReactiveNative.Shared.Tests to the ReactNative.Net46.Tests projects.

A handful of tests in JavaScriptModuleRegistryTests.cs utilized NMock. A specialized class inheriting from JavaScriptModuleRegistryTests was added to the ReactNative.Net46.Tests project.  The specialized class "JavaScriptModuleRegistryNMockTests" contains only the NMock dependent tests and the base class contains all tests that can be shared.

All references to NMock have been removed from the test code in the ReactNative.Shared.Tests projects...

A reference to the ReactNative.Shared.Tests has been added to the ReactNative.Tests project.   Additionally, the NUnit v3.41 nuGet package and the NUnit3TestAdapter v 3.5 nuGet package were added to the ReactNative.Tests project.

The NUnit3TestAdapter v 3.41 nuGet package could not be loaded in the ReactNative.Tests project so v3.5 (the latest) was used. This creates a version mismatch with the NUnit package and the nuGet packages used in the ReactNative.Net46.Tests project...
…t is already included in ReactNative.Shared.Tests
…egistryNMockTests.cs" to match the class name...
@matthargett matthargett changed the title Nmock net46 Re-enablle Shared tests for the Universal build Oct 29, 2016
@matthargett matthargett changed the title Re-enablle Shared tests for the Universal build Re-enable Shared tests for the Universal build Oct 29, 2016
Copy link
Contributor

@matthargett matthargett left a comment

Choose a reason for hiding this comment

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

Need to upgrade NUnit nuget references in ReactNative.Net46.Tests project to also be NUnit 3.5.0.

I verified ReactNative.Tests does indeed include the Shared tests, but VS2015 Test Explorer doesn't show two separate tests with the same name as being separate unless you choose 'group by project' in the drop down in the top left of the VS2015 Test Explorer.

I tried various ways to get before-and-after code coverage on Universal tests, but after 5 different tools and much trial and error, I gave up. Maybe code coverage works in Visual Studio Enterprise edition, but I don't have access to that right now. I was able to verify coverage on Net46.Tests didn't go down.

I also found a few bugs in ReSharper's test runner, not the fault of this commit, which I filed with JetBrains.

@rozele
Copy link
Collaborator

rozele commented Oct 31, 2016

��<�!�D�O�C�T�Y�P�E� �h�t�m�l�>�

This file shouldn't be added (we may want to add it to the .gitignore while we're at it)


Refers to: ReactWindows/UpgradeLog.htm:1 in 53461be. [](commit_id = 53461be, deletion_comment = False)

@rozele
Copy link
Collaborator

rozele commented Oct 31, 2016

So have the JavaScriptModuleBaseTests been lost from UWP? #Closed


Refers to: ReactWindows/ReactNative.Tests/ReactNative.Tests.csproj:97 in 53461be. [](commit_id = 53461be, deletion_comment = False)


namespace ReactNative.Tests
{
class MockJavaScriptExecutor : IJavaScriptExecutor
Copy link
Collaborator

@rozele rozele Oct 31, 2016

Choose a reason for hiding this comment

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

MockJavaScriptExecutor [](start = 10, length = 22)

You may have to leave this if you are going to bring back the JavaScriptModuleBaseTests that were available to UWP before. #Closed

Copy link
Contributor

@matthargett matthargett Oct 31, 2016

Choose a reason for hiding this comment

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

MockJavaScriptExecutor comes in from Shared.Tests now. We had made a copy of it in Shared.Tests in an earlier PR leading up to this. A few other fixtures use that static mock as well. #Closed

@matthargett
Copy link
Contributor

(I can't reply to the specific comment, for some reason.) Some tests in JavaScriptModuleBaseTests use NMock instead of the hand-rolled mocks there previously, so they're now in Net46. I think it would be a good idea to extract a base class into Shared.Tests and sub-class it in Net46 to add the complex interaction tests there. If we want to back-fill the hand-rolled mocks to improve coverage in Universal tests, we can do that in a subclass in that project. What do you think of that @rozele ?


npm install -g react-native-cli

npm install
Copy link
Collaborator

Choose a reason for hiding this comment

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

This step will take a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid of it, but caching node_modules with AppVeyor brings it down. Happy to ditch it for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's ditch it until we have something like Appium in place.


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

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we did Yarn? It does system caching automatically.


mkdir ReactWindows\Playground\ReactAssets

react-native bundle --platform windows --entry-file ReactWindows/Playground/index.windows.js --bundle-output ReactWindows/Playground/ReactAssets/index.windows.bundle --assets-dest ReactWindows/Playground/ReactAssets/
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to test this? The bundle command comes exclusively from facebook/react-native

Copy link
Contributor

Choose a reason for hiding this comment

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

I had it in my build for two reasons:

  • To make sure upstream republishes with same version didn't become incompatible with the Playground index.*.js
  • To have Playground ReleaseBundle artefacts at the end of the build that were ready to run for basic UI tests that would verify the "Welcome to React Native" message (versus a redbox, which would be a regression).

Happy to ditch the step for the main repo build.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's only add this back once we actually have an Appium step. All this is doing for now is building the bundle, which is pretty fail safe (it won't catch most of the issues that we'd actually care about for regressions that we could only see by running an app using the bundle)


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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unfortunately, JavaScript is not compiled, and the bundler only picks up very specific "compile-like" issues like missing modules


In reply to: 85981159 [](ancestors = 85981159,85962148)

}

[Test]
public void InvokesReactInstanceWhenFetchedModuleIsCalled()
Copy link
Collaborator

@rozele rozele Nov 1, 2016

Choose a reason for hiding this comment

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

InvokesReactInstanceWhenFetchedModuleIsCalled [](start = 20, length = 45)

I presume a duplicate of this is left in ReactNative.Tests? #Closed

Copy link
Contributor

@matthargett matthargett Nov 1, 2016

Choose a reason for hiding this comment

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

No, but we can fix that. Can you fix and push @pre10der89 ? #Closed

Copy link
Contributor Author

@pre10der89 pre10der89 Nov 1, 2016

Choose a reason for hiding this comment

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

My latest push adds the InvokesReactInstanceWhenFetchedModuleIsCalled and ThrowsWhenUnkonwModuleRequested to the ReactNative.Tests using the implementations that existed before they were converted to NMock... #Closed

@rozele
Copy link
Collaborator

rozele commented Nov 1, 2016

Seems weird to have base classes for unit test classes.


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

…and the containing

  class from JavascriptModuleRegistryTests to JavascriptModuleRegistrySharedTests.

* ReactNative.Net46.Tests: Renamed JavascriptModuleRegistryNMockTests.cs and the containing
  class from JavascriptModuleRegistryNMockTests to JavascriptModuleRegistryTests. Changed the base class to
  JavascriptModuleRegistrySharedTests.

* ReactNative.Tests:  Added JavascriptModuleRegistryTests.cs to project that uses Univesal-specific mocking in place
  of NMock.  Inherited containing class from JavascriptModuleRegistrySharedTests.
…ecific tests in JavaScriptModuleRegistryTests
@matthargett
Copy link
Contributor

Couple of different texts for the test base class pattern: Test Driven Development, A Practical Guide; Extreme Programming Adventures in C#; Test-Driven Development in Microsoft .NET; Pragmatic Unit Testing in C#. Even in some of the React projects I've worked on, an inheritance pattern is used in Selenium-based Jasmine tests when mixins aren't appropriate.

%DEVENV% /build "Debug|x64" ReactWindows\ReactNative.sln

npm install -g npm

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needed?

npm install -g npm

npm version

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope

set DEVENV="%VS140COMNTOOLS%\..\IDE\devenv"

%DEVENV% /build "Debug|x64" ReactWindows\ReactNative.sln

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we were going to do Debug builds for all platforms, release for just x64.


npm version

%DEVENV% /build "ReleaseBundle|x64" ReactWindows\ReactNative.sln
Copy link
Collaborator

Choose a reason for hiding this comment

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

ReleaseBundle [](start = 21, length = 13)

I don't think we really need to build the *Bundle configs for react-native-windows, basically because the Bundle part only comes in to play at the application level (ReleaseBundle just maps to Release and DebugBundle just maps to Debug)

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a couple of #ifs in the code for bundle builds that I'd like to keep coverage on. We can do it for x86 Debug instead, perhaps?

@rozele
Copy link
Collaborator

rozele commented Nov 2, 2016

It looks like there is a change to the pointer for the Examples branch. It might have been committed by mistake.

@rozele rozele merged commit fb20e5a into microsoft:master Nov 2, 2016
rozele pushed a commit that referenced this pull request Nov 10, 2016
* The NMock3 library is not compatiable with Universal Windows Projects... Any TestFixtures that utilize the NMock3 library need to be isolated in the .NET test project(s)...

Moved NMock3 related tests from ReactiveNative.Shared.Tests to ReactNative46.Tests.

The majority of tests in JavaScriptModuleBaseTests.cs utilized NMock so the entire file was moved from the ReactiveNative.Shared.Tests to the ReactNative.Net46.Tests projects.

A handful of tests in JavaScriptModuleRegistryTests.cs utilized NMock. A specialized class inheriting from JavaScriptModuleRegistryTests was added to the ReactNative.Net46.Tests project.  The specialized class "JavaScriptModuleRegistryNMockTests" contains only the NMock dependent tests and the base class contains all tests that can be shared.

All references to NMock have been removed from the test code in the ReactNative.Shared.Tests projects...

A reference to the ReactNative.Shared.Tests has been added to the ReactNative.Tests project.   Additionally, the NUnit v3.41 nuGet package and the NUnit3TestAdapter v 3.5 nuGet package were added to the ReactNative.Tests project.

The NUnit3TestAdapter v 3.41 nuGet package could not be loaded in the ReactNative.Tests project so v3.5 (the latest) was used. This creates a version mismatch with the NUnit package and the nuGet packages used in the ReactNative.Net46.Tests project...

* The NMock3 library is not compatiable with Universal Windows Projects... Any TestFixtures that utilize the NMock3 library need to be isolated in the .NET test project(s)...

Moved NMock3 related tests from ReactiveNative.Shared.Tests to ReactNative46.Tests.

The majority of tests in JavaScriptModuleBaseTests.cs utilized NMock so the entire file was moved from the ReactiveNative.Shared.Tests to the ReactNative.Net46.Tests projects.

A handful of tests in JavaScriptModuleRegistryTests.cs utilized NMock. A specialized class inheriting from JavaScriptModuleRegistryTests was added to the ReactNative.Net46.Tests project.  The specialized class "JavaScriptModuleRegistryNMockTests" contains only the NMock dependent tests and the base class contains all tests that can be shared.

All references to NMock have been removed from the test code in the ReactNative.Shared.Tests projects...

A reference to the ReactNative.Shared.Tests has been added to the ReactNative.Tests project.   Additionally, the NUnit v3.41 nuGet package and the NUnit3TestAdapter v 3.5 nuGet package were added to the ReactNative.Tests project.

The NUnit3TestAdapter v 3.41 nuGet package could not be loaded in the ReactNative.Tests project so v3.5 (the latest) was used. This creates a version mismatch with the NUnit package and the nuGet packages used in the ReactNative.Net46.Tests project...

* Removing "MockJavaScriptExecutor.cs" from ReactNative.Tests because it is already included in ReactNative.Shared.Tests

* Renaming "JavaScriptModuleRegistryMockTests.cs" to "JavaScriptModuleRegistryNMockTests.cs" to match the class name...

* Renaming JavaScriptModuleRegistryMockTests.cs to match the included class name...

* Upgrade NUnit version in ReactNative.Tests project

* Upgrade NUnit nuget references in ReactNative.Net46.Tests project to also be NUnit 3.5.0.

NUnit upgraded to 3.5.0
NUnit3TestRunner upgraded to 3.5.1

* Syncing with upstream

* Removing UpgradeLog.htm;  adding UpgradeLog.htm to .gitignore

* Adding appveyor.yml

* * ReactNative.Shared.Tests:  Renamed JavascriptModuleRegistryTests.cs and the containing
  class from JavascriptModuleRegistryTests to JavascriptModuleRegistrySharedTests.

* ReactNative.Net46.Tests: Renamed JavascriptModuleRegistryNMockTests.cs and the containing
  class from JavascriptModuleRegistryNMockTests to JavascriptModuleRegistryTests. Changed the base class to
  JavascriptModuleRegistrySharedTests.

* ReactNative.Tests:  Added JavascriptModuleRegistryTests.cs to project that uses Univesal-specific mocking in place
  of NMock.  Inherited containing class from JavascriptModuleRegistrySharedTests.

* Adding new and renamed files

* Restoring the 0.27-stable branch implementations of some universal specific tests in JavaScriptModuleRegistryTests

* Removing npm install -g react-native-cli and react-native bundle components from the AppVeyor config

* Removing x86 and ARM builds and x86 tests from AppVeyor config

* Reverting to previous examples version
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.

None yet

4 participants