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

exclude mos from file #160

Merged
merged 16 commits into from
May 8, 2017
Merged

exclude mos from file #160

merged 16 commits into from
May 8, 2017

Conversation

thorade
Copy link
Contributor

@thorade thorade commented May 5, 2017

This adds a new function that can populate the global _excludeMos list from a file.
This closes #143.

@@ -0,0 +1,7 @@
# skip some mos tests
Copy link
Member

Choose a reason for hiding this comment

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

What is this file doing? I don't see it anywhere else used in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used it locally to test whether the new function works, will write a proper test instead and use it there.

counter += 1
return counter

def setExcludeMosFromFile(self, excludeFile=None):
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you allow excludeFile=None in which case the function does nothing. I don't think users will call a function to exclude files, and expect the function to do nothing?

Also, this function is not used in test_development_regressiontest.py. Please add so it becomes part of the testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure I undestand the first comment. Should I just remove the None? The function can currently only add things to the list, should it be renamed?

else:
print("*** Warning: Excluded file {} from the regression tests.".format(fileName))
if filNam in self._excludeMos:
print("*** Warning: Excluded file {} from the regression tests.".format(filNam))
Copy link
Member

Choose a reason for hiding this comment

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

Use self._reporter.writeWarning rather than write to console, otherwise the console and unitTest.log will show different things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That line was just copied, I will change it to write to the log file.

Copy link
Member

@mwetter mwetter left a comment

Choose a reason for hiding this comment

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

@thorade : Please see comments

@thorade
Copy link
Contributor Author

thorade commented May 6, 2017

All comments addressed

@mwetter
Copy link
Member

mwetter commented May 8, 2017

@thorade I changed the following:

  • I renamed setExcludeMosFromFile to setExcludeTest as in the future, we may want to specify tests not only in mos file but maybe in a higher level format so we can generate tests for different simulators.
  • Consequently, I renamed _excludeMos to _exclude_tests

I will merge when the tests pass.

@mwetter mwetter merged commit 2a19fdc into master May 8, 2017
@mwetter mwetter deleted the issue143_setExcludeMosFromFile branch May 8, 2017 18:11
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.

option to skip .copiedFiles in regression test
2 participants