Migrate Unit Tests to use DataRow #186

josesimoes · 2022-06-03T17:58:59Z

Description

Migrate UnitTestFormat.
Update test framework sub-module.

Motivation and Context

Addressing Migrate Unit Tests to DataRow Home#1158.

How Has This Been Tested?

Screenshots

Types of changes

Improvement (non-breaking change that improves a feature, code or algorithm)
Bug fix (non-breaking change which fixes an issue with code or algorithm)
New feature (non-breaking change which adds functionality to code)
Breaking change (fix or feature that would cause existing functionality to change)
Config and build (change in the configuration and build system, has no impact on code or features)
Dependencies (update dependencies and changes associated, has no impact on code or features)
Unit Tests (add new Unit Test(s) or improved existing one(s), has no impact on code or features)
Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist:

My code follows the code style of this project (only if there are changes in source code).
My changes require an update to the documentation (there are changes that require the docs website to be updated).
I have updated the documentation accordingly (the changes require an update on the docs in this repo).
I have read the CONTRIBUTING document.
I have tested everything locally and all new and existing tests passed (only if there are changes in source code).
I have added new tests to cover my changes.

- Migrate UnitTestFormat. - Update test framework sub-module.

josesimoes · 2022-06-03T18:01:53Z

@torbacz I couldn't resist and I've started the migration of the Unit Tests to use the shiny new DataRow 😉

The thing is that it's failing...

Care to take a look please?

torbacz · 2022-06-03T18:08:13Z

@josesimoes
Will do, probably on Monday.

torbacz · 2022-06-04T17:39:32Z

@josesimoes
I think I found the problem. Test runner is returning data in "StringFormat_02 - params[...], 123" format and there is a comma in that format. Also in your parameters, there is a comma in one of parameter. I've run into similar issue in one of my own nano project.
So we need to new way to identify DataRow individual test cases (currently done by passed parameters converted to string). I was thinking about doing it by order, by then we will lose easy way to identify which test is failing (new name would be ex. StringFormat_02_1). I need to find a new way to implement that.

Also I found out 2 more issues:

When using long string data - it's failing
When we add reference into "NFApp" project (the one containing static main method) the test runner is invoking main program instead of tests.

On Monday I'm going to create all 3 issues and try to resolve the first one (which is most important for now)

josesimoes · 2022-06-04T17:58:51Z

OK... I'm not looking at the code right now so I have little advice to offer at this time. Maybe we can switch the separator from a comma to another char less likely to be found im those contexts like a pipe | or an asterisk * or a #.

torbacz · 2022-06-06T11:22:48Z

Tests/NFUnitTestArithmetic/UnitTestFormat.cs


+        [TestMethod]


You can remove [TestMethod] parameter when using [DataRow] :)

🤔 maybe I could... but I don't think we should... Check here please.

The official usage guidance calls for both. I get it that we may handle this differently but then we have to take care of it internally so it doesn't break the official usage.

Maybe process DataRow before TestMethod and then don't call it "a second time" or make a mutually exclusive check... not sure what would be more efficient.

With current implementation of DataRow - it will probably fail. Test framework will try to invoke method and pass "null" as parameters.

" mutually exclusive check" - do you mean something like this:
if (testMethod.Contains(DataRowAttribute) && test.Method.Contains(TestMethodAttrubite)
{
throw Exception();
}

Or just ignore TestMethod attribute when there is DataRow?

I would go with the latter. Test will be executed (as intended) just that with the correct context (using DataRow(s)). And the "doomed to fail" execution of TestMethod is skipped.

torbacz · 2022-06-06T15:13:51Z

@josesimoes
Is there a bug in string format? I've copied test from your code and it's failing on assertion.

[DataRow("Right align in 10 chars: {0,10:N2}: and then more", 1234.5641, "Right align in 10 chars: 1,234.56 and then more")]
public void StringFormat_02(string formatString, double value, string outcomeMessage)
{
    Console.WriteLine(formatString);
    Console.WriteLine(outcomeMessage);

    var stringFormatValue = string.Format(formatString, value);
    Console.WriteLine(stringFormatValue);

    // Test alignment operator which is the "," and a number. Negative is right aligned, positive left aligned
    Assert.Equal(stringFormatValue, outcomeMessage);
}

Output from console:

Message: 
Right align in 10 chars:   1,234.56: and then more is not equal to Right align in 10 chars: 1

  Standard Error: 
Right align in 10 chars: {0,10:N2}: and then more
Right align in 10 chars: 1,234.56 and then more
Right align in 10 chars:   1,234.56: and then more
    ++++ Exception System.Exception - 0x00000000 (1) ++++
    ++++ Message: Right align in 10 chars:   1,234.56: and then more is not equal to Right align in 10 chars: 1,234.56 and then
 more. 
    ++++ nanoFramework.TestFramework.Assert::Equal [IP: 002f] ++++
    ++++ nanoFramework.TestFramework.Test.TestOfDataRow::StringFormat_02 [IP: 0028] ++++
    ++++ System.Reflection.MethodBase::Invoke [IP: 0000] ++++
    ++++ nanoFramework.TestFramework.UnitTestLauncher::RunTest [IP: 0070] ++++
    ++++ nanoFramework.TestFramework.UnitTestLauncher::Main [IP: 0093] ++++

Note: Additional spaces and :

josesimoes · 2022-06-06T15:19:19Z

@josesimoes Is there a bug in string format? I've copied test from your code and it's failing on assertion.

Not that I'm aware of... 😯 unit test have been passing in mscorlib.
You can check that easily if you temporarily revert these change on StringFormat and DataRow and run the previous version.

torbacz · 2022-06-06T15:45:34Z

Found it, when coping from github it removed 2 spaces from parameters....

PR for new version of test framework on the way - resolving ',' in parameters, long strings in parameters and skipping "TestMethod" if "DataRow" is used.

torbacz · 2022-06-06T16:49:49Z

@josesimoes

Working now :)

josesimoes · 2022-06-06T16:54:22Z

@torbacz brilliant! Thank you very much for the quick turn around.
Care to commit your changes to the [update-test-framework] branch? seems a waste not doing it... 😉

torbacz · 2022-06-06T17:14:08Z

@josesimoes
Can't, got 403 error although I've joined nano organization

Remote: Permission to nanoframework/CoreLibrary.git denied to torbacz.
Error encountered while pushing to the remote repository: Git failed with a fatal error.
Git failed with a fatal error.
unable to access 'https://github.com/nanoframework/CoreLibrary/': The requested URL returned error: 403

torbacz · 2022-06-06T17:17:26Z

I've change only 2 things in your code:

Submodule updated to latest master (why it's event there? Wouldn't It be easier just to use package?)
Update test nuget package via VS

josesimoes · 2022-06-06T17:17:55Z

@torbacz I had to wait for you to join the organization before assigning the permissions. You can now push that branch.

torbacz · 2022-06-06T17:19:37Z

@josesimoes
Still rejected, this time

remote: error: GH006: Protected branch update failed for refs/heads/update-test-framework.        
remote: error: At least 1 approving review is required by reviewers with write access.        
error: failed to push some refs to 'https://github.com/nanoframework/CoreLibrary'
To https://github.com/nanoframework/CoreLibrary
!	refs/heads/update-test-framework:refs/heads/update-test-framework	[remote rejected] (protected branch hook declined)
Done

josesimoes · 2022-06-06T17:20:16Z

Submodule updated to latest master (why it's event there? Wouldn't It be easier just to use package?)

Please DO NOT change that otherwise it wont' work! 😱
This is because this is mscorlib, of which the test framework depends on. It's an "inception" thing.

josesimoes · 2022-06-06T17:21:47Z

@torbacz it should allow you to push now.

torbacz · 2022-06-06T17:23:56Z

@josesimoes
Same error.

Btw "Do not change that" - do not change to package or do not update submodule to latest?

josesimoes · 2022-06-06T17:26:13Z

@josesimoes Same error.

Odd... Please go for a PR against that branch instead.

josesimoes · 2022-06-06T17:27:04Z

Btw "Do not change that" - do not change to package or do not update submodule to latest?

OK to update sub-module.
NOT OK to switch to package instead of direct reference.

…nto update-test-framework

josesimoes · 2022-09-28T12:05:06Z

@torbacz I'm thinking that it will take a while to complete the migration to DataRow (considering the lack of participation from the community). Therefore I suggest that this is merged as it is and the work can be resumed in future iterations.

torbacz · 2022-09-28T13:05:53Z

Ok! Maybe create an issue for it?

josesimoes · 2022-09-28T13:15:37Z

Maybe create an issue for it?

Good idea! It will add visibility to the task. Care to do add it and link it back to this PR?

Migrate Unit Tests to use DataRow

7c1f967

- Migrate UnitTestFormat. - Update test framework sub-module.

nfbot added Type: dependencies Type: Unit Tests labels Jun 3, 2022

josesimoes marked this pull request as draft June 3, 2022 17:59

torbacz reviewed Jun 6, 2022

View reviewed changes

torbacz and others added 3 commits June 6, 2022 19:46

Update to latest test framework

6ca28d8

Merge branch 'main' of https://github.com/nanoframework/CoreLibrary i…

92566c1

…nto update-test-framework

Merge branch 'main' into update-test-framework

00d9a92

torbacz approved these changes Sep 29, 2022

View reviewed changes

torbacz marked this pull request as ready for review September 29, 2022 17:36

torbacz mentioned this pull request Sep 29, 2022

Migrate Unit Tests to DataRow nanoframework/Home#1158

Closed

josesimoes mentioned this pull request Sep 29, 2022

Migrate Unit Tests to DataRow nanoframework/Home#1159

Open

josesimoes merged commit 76bb301 into main Sep 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate Unit Tests to use DataRow #186

Migrate Unit Tests to use DataRow #186

josesimoes commented Jun 3, 2022 •

edited

Loading

josesimoes commented Jun 3, 2022

torbacz commented Jun 3, 2022

torbacz commented Jun 4, 2022

josesimoes commented Jun 4, 2022

torbacz Jun 6, 2022

josesimoes Jun 6, 2022

torbacz Jun 6, 2022

josesimoes Jun 6, 2022

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

torbacz commented Jun 6, 2022

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

torbacz commented Jun 6, 2022

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022 •

edited

Loading

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Jun 6, 2022 •

edited

Loading

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Sep 28, 2022

torbacz commented Sep 28, 2022

josesimoes commented Sep 28, 2022 •

edited

Loading

Migrate Unit Tests to use DataRow #186

Migrate Unit Tests to use DataRow #186

Conversation

josesimoes commented Jun 3, 2022 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots

Types of changes

Checklist:

josesimoes commented Jun 3, 2022

torbacz commented Jun 3, 2022

torbacz commented Jun 4, 2022

josesimoes commented Jun 4, 2022

torbacz Jun 6, 2022

Choose a reason for hiding this comment

josesimoes Jun 6, 2022

Choose a reason for hiding this comment

torbacz Jun 6, 2022

Choose a reason for hiding this comment

josesimoes Jun 6, 2022

Choose a reason for hiding this comment

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

torbacz commented Jun 6, 2022

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

torbacz commented Jun 6, 2022

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022 • edited Loading

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Jun 6, 2022 • edited Loading

torbacz commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Jun 6, 2022

josesimoes commented Sep 28, 2022

torbacz commented Sep 28, 2022

josesimoes commented Sep 28, 2022 • edited Loading

josesimoes commented Jun 3, 2022 •

edited

Loading

josesimoes commented Jun 6, 2022 •

edited

Loading

josesimoes commented Jun 6, 2022 •

edited

Loading

josesimoes commented Sep 28, 2022 •

edited

Loading