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

Unify mbed OS tools testing style with what's used in mbed-ls, htrun, greentea #4984

Merged
merged 10 commits into from
Sep 6, 2017

Conversation

theotherjimmy
Copy link
Contributor

@theotherjimmy theotherjimmy commented Aug 28, 2017

Also enables coverage numbers!

Converts our usage of testing frameworks to use py.test.

Ready

This should only need to be tested by travis CI.

@theotherjimmy
Copy link
Contributor Author

@0xc0170 Could you take a look? @adbridge Could you take a look as well?

@0xc0170
Copy link
Contributor

0xc0170 commented Aug 31, 2017

Don't be surprised if this breaks travis CI.

Is this still in devel? I'll have a look tomorrow.

@theotherjimmy
Copy link
Contributor Author

Actually, no. It's all working now. I'll update the description.

root_dir = abspath(dirname(__file__))

@pytest.mark.parametrize("name", filter(lambda d: is_test(join(root_dir, d)),
os.listdir(root_dir)))
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting here is a bit weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it is. I'll have to get on that.

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 has been deleted in this branch....

@@ -0,0 +1,12 @@

{
"f": {
Copy link
Contributor

Choose a reason for hiding this comment

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

What are 'f' and 'b2' ? Don't seem very meaningful names ?

@@ -0,0 +1,6 @@

{
Copy link
Contributor

Choose a reason for hiding this comment

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

Spurious space ?

@@ -0,0 +1,6 @@

{
Copy link
Contributor

Choose a reason for hiding this comment

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

spurious space?


expected_results = {
{
Copy link
Contributor

Choose a reason for hiding this comment

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

spurious space

when both are specified

:param mock_json_file_to_dict: mock of function json_file_to_dict
:param _: patch of function _process_config_and_overrides (not tested)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing target param ?

for target in named_targets:
assert target.device_name in cache.index,\
("Target %s contains invalid device_name %s" %
(target.name, target.device_name))
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment

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's actually also correct.


@contextmanager
def temp_target_file(extra_target, json_filename='custom_targets.json'):
"""Create an extra targets temp file in a context manager"""
Copy link
Contributor

Choose a reason for hiding this comment

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

parameters ?

"""
Test find_tests for correct use of app_config
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter descriptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@pytest.mark.parametrize("app_config", ["app_config", None])
def test_find_tests_app_config(build_path, target, toolchain_name, app_config):
"""
Test find_tests for correct use of app_config
Copy link
Contributor

Choose a reason for hiding this comment

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

parameter descriptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 4, 2017

@adbridge Please rereview

@@ -191,8 +201,10 @@ def test_generate_output_csv_ci(memap_parser, tmpdir, depth):
"""
Test ensures that an output of type "csv-ci" can be generated correctly

:return:
:param memap_parser: Mocked parser
:param tmpdir: a temporary directory
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to say why there is even a temporary directory? Otherwise not that helpful a parameter description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes of course.

@@ -179,8 +186,11 @@ def test_generate_output_table(memap_parser, depth):
def test_generate_output_json(memap_parser, tmpdir, depth):
"""
Test that an output of type "json" can be generated correctly
:param memap_parser: Mocked parser
:param tmpdir: a temporary directory
Copy link
Contributor

Choose a reason for hiding this comment

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

see comment below

@theotherjimmy
Copy link
Contributor Author

@adbridge I think I addressed your review comments. Could you take a look?

Wipe out all blank lines in json:
  find tools/test -name 'test_data.json' | xargs sed -i -e '/^$/d'
Move all start braces back a space:
  find tools/test -name 'test_data.json' | xargs sed -i -e "s/^ {/{/"
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 399b6ac on theotherjimmy:unify-unit-tests into ** on ARMmbed:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9d0e8ab on theotherjimmy:unify-unit-tests into ** on ARMmbed:master**.

@theotherjimmy
Copy link
Contributor Author

Woooooo! We only need to run Travis on this one (it only affects the unit tests) and Travis passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants