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

`./mach test --all` passes incorrect number of arguments to test-tidy #25554

Closed
warren-fisher opened this issue Jan 19, 2020 · 1 comment
Closed

Comments

@warren-fisher
Copy link
Contributor

@warren-fisher warren-fisher commented Jan 19, 2020

On Ubuntu 19.10, ./mach test --all fails on the current master branch with:

Error running mach:

    ['test', '--all']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.
You can invoke |./mach busted| to check if this issue is already on file. If it
isn't, please use |./mach busted file| to report it. If |./mach busted| is
misbehaving, you can also inspect the dependencies of bug 1543241.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

TypeError: test_tidy() takes exactly 7 arguments (5 given)

  File "/home/warren/programming/servo/python/servo/testing_commands.py", line 176, in test
    Registrar.dispatch("test-%s" % suite, context=self.context, **kwargs)
  File "/home/warren/programming/servo/python/_virtualenv2.7/lib/python2.7/site-packages/mach/registrar.py", line 152, in dispatch
    return self._run_command_handler(handler, context=context, **kwargs)
  File "/home/warren/programming/servo/python/_virtualenv2.7/lib/python2.7/site-packages/mach/registrar.py", line 109, in _run_command_handler
    result = fn(**kwargs)

#16071 seems related. The main file in question is python/servo/testing_commands.py.

Running ./mach test-tidy --self-test succeeds with all 29 tests passing.

Running ./mach test --all --self-test or ./mach test tidy --self-test fail with the same error as ./mach test --all. Perhaps --self-test was never meant to work with either of these options, I am not sure.

@warren-fisher
Copy link
Contributor Author

@warren-fisher warren-fisher commented Jan 19, 2020

After further testing (adding a print statement on line 175 of python/servo/testing_commands the following dictionary is passed to test_tidy() in python/servo/testing_commands.py as keyword arguments:
{u'all_files': False, u'self_test': True, u'no_progress': False, u'stylo': False}

As can be seen the no_wpt and force_cpp arguments are not being provided. However using ./mach test-tidy these are passed using the @CommandArgument decorators default value. Perhaps it would be best to set no_wpt and force_cpp to have defaults of False in the function signature. This should allow for ./mach test --all to correctly identify the default values intended by the @CommandArgument decorator without have to change anything else.

Testing this on my local machine did fix the problem. Will submit a PR tonight.

bors-servo added a commit that referenced this issue Jan 20, 2020
Add default arguments so that ./mach test --all works

<!-- Please describe your changes on the following line: -->
Fix `./mach test --all` not getting any default values for `force_cpp` and `no_wpt` in [python/servo/testing_commands.py](https://github.com/servo/servo/blob/2a594821ba44ba2c13e9a39c6fd8c5a450bd06f4/python/servo/testing_commands.py#L319-L334). For `./mach test-tidy` the default values are obtained using the `@CommandArgument` decorator, but this does not apply for calling `test_tidy()` when using `./mach test --all` (which calls the function directly). This call is found on line 109 of `registrar.py` which should be located in something like `python/_virtualenv2.7/python2.7/site-packages/mach/registrar.py` (but not located in this git repository).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25554 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because the change is minimal and non breaking. Any existing code should be able to work if it uses keyword arguments because the default values will be overriden. If there is code that calls `test_tidy()` with arguments by position this would cause breakage, but I do not believe this happens anywhere in the `testing_command.py` file. `./mach test-tidy` should still work because the arguments are set using the `@CommandArgument` decorator.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 20, 2020
Add default arguments so that ./mach test --all works

<!-- Please describe your changes on the following line: -->
Fix `./mach test --all` not getting any default values for `force_cpp` and `no_wpt` in [python/servo/testing_commands.py](https://github.com/servo/servo/blob/2a594821ba44ba2c13e9a39c6fd8c5a450bd06f4/python/servo/testing_commands.py#L319-L334). For `./mach test-tidy` the default values are obtained using the `@CommandArgument` decorator, but this does not apply for calling `test_tidy()` when using `./mach test --all` (which calls the function directly). This call is found on line 109 of `registrar.py` which should be located in something like `python/_virtualenv2.7/python2.7/site-packages/mach/registrar.py` (but not located in this git repository).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25554 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because the change is minimal and non breaking. Any existing code should be able to work if it uses keyword arguments because the default values will be overriden. If there is code that calls `test_tidy()` with arguments by position this would cause breakage, but I do not believe this happens anywhere in the `testing_command.py` file. `./mach test-tidy` should still work because the arguments are set using the `@CommandArgument` decorator.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Jan 20, 2020
Add default arguments so that ./mach test --all works

<!-- Please describe your changes on the following line: -->
Fix `./mach test --all` not getting any default values for `force_cpp` and `no_wpt` in [python/servo/testing_commands.py](https://github.com/servo/servo/blob/2a594821ba44ba2c13e9a39c6fd8c5a450bd06f4/python/servo/testing_commands.py#L319-L334). For `./mach test-tidy` the default values are obtained using the `@CommandArgument` decorator, but this does not apply for calling `test_tidy()` when using `./mach test --all` (which calls the function directly). This call is found on line 109 of `registrar.py` which should be located in something like `python/_virtualenv2.7/python2.7/site-packages/mach/registrar.py` (but not located in this git repository).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #25554 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because the change is minimal and non breaking. Any existing code should be able to work if it uses keyword arguments because the default values will be overriden. If there is code that calls `test_tidy()` with arguments by position this would cause breakage, but I do not believe this happens anywhere in the `testing_command.py` file. `./mach test-tidy` should still work because the arguments are set using the `@CommandArgument` decorator.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

1 participant
You can’t perform that action at this time.