Skip to content
This repository has been archived by the owner on Sep 18, 2024. It is now read-only.

Refactor code hierarchy part 3: Unit test #3037

Merged
merged 3 commits into from
Oct 30, 2020
Merged

Conversation

liuzhe-lz
Copy link
Contributor

@liuzhe-lz liuzhe-lz commented Oct 27, 2020

This PR fixes most unit test cases broken by code hierarchy refactor, on Linux and macOS.

All Python UT are moved to test/ut folder. Run them with:

$ cd test
$ pytest ut

The new UT pipeline is placed in pipelines directory, and I will put future v2.0 pipelines there too.
One important reason that I changed pipeline location is to make the transition smooth.
Once pipeline porting is done, we may either move them back to old location or just remove the old ones. This can be discussed later.

There are several broken test cases which need further investigation. You can search FIXME in test/ut folder to find them.
UT on Windows is more problematic and is supposed to be fixed later. In fact it was not fully tested in v1.x versions.

@liuzhe-lz liuzhe-lz mentioned this pull request Oct 28, 2020
77 tasks
@@ -5,6 +5,10 @@
/test/model_path/
/test/temp.json
/test/ut/sdk/*.pth
/test/ut/tools/annotation/_generated/
/ts/nni_manager/exp_profile.json
/ts/nni_manager/metrics.json
Copy link
Contributor

Choose a reason for hiding this comment

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

why add this 3 json files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are generated during unit test.

@@ -0,0 +1,216 @@
# To reduce debug cost, steps are sorted differently on each platform,
Copy link
Contributor

@SparkSnail SparkSnail Oct 28, 2020

Choose a reason for hiding this comment

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

can't get the point, are these test cases in this file duplicated with other pipelines? when will we use this test file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is basically azure-pipelines.yml. I'm trying to do some refactor and don't want to block other PRs during development. When refactor is done, I will replace the old pipeline with this one.

# if fc is not None:
# fc.close()

#def test_send_multi_node(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

why annotate these functions?

Copy link
Contributor Author

@liuzhe-lz liuzhe-lz Oct 28, 2020

Choose a reason for hiding this comment

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

Because this test case failed and I can't fix it.
As stated in FIXME, BaseChannel.send() will raise if the second argument is a string, contradicting to it's doc, and this test case calls it with fc1.send(CommandType.ReportGpuInfo, "command1").
I think this test case was skipped in old pipeline for unknown reason.

@liuzhe-lz liuzhe-lz merged commit bc0f8f3 into microsoft:master Oct 30, 2020
chicm-ms pushed a commit to chicm-ms/nni that referenced this pull request Nov 9, 2020
@liuzhe-lz liuzhe-lz deleted the v2-p3 branch November 23, 2020 03:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants