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

Add a mach command to test WebIDL.py #9397

Closed
psdh opened this issue Jan 21, 2016 · 30 comments
Closed

Add a mach command to test WebIDL.py #9397

psdh opened this issue Jan 21, 2016 · 30 comments

Comments

@psdh
Copy link
Contributor

@psdh psdh commented Jan 21, 2016

We should run runtests.py.
You could possibly name it mach test-webidl

@jdm
Copy link
Member

@jdm jdm commented Jan 21, 2016

This would belong in python/testing_commands.py.

@nox
Copy link
Member

@nox nox commented Jan 22, 2016

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 23, 2016

I'd like to do this.

@jdm jdm added the C-assigned label Jan 23, 2016
@jdm
Copy link
Member

@jdm jdm commented Jan 23, 2016

@therealkbhat It's yours! Let us know if you've got any questions!

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 24, 2016

We probably should update update.sh to make it update the tests from mozilla-central too.

Isn't it already fetching from mozilla-central?

@psdh
Copy link
Contributor Author

@psdh psdh commented Jan 24, 2016

@therealkbhat Note that it just fetches the WebIDL.py file. nox was talking updating the tests dir too

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 24, 2016

@psdh ah, right.

Also, I'm having trouble putting my finger on how exactly I need to run runtests.py. Here's what I have so far: gist.

@nxnfufunezn
Copy link
Contributor

@nxnfufunezn nxnfufunezn commented Jan 24, 2016

I guess putting the paren in print will solve the issue..

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 24, 2016

@nxnfufunezn that worked. :)

Relative imports is throwing me off, though. It says ImportError: No module named WebIDL.
Is this because runtests.py is running in the context of testing_commands.py, so it doesn't see WebIDL.py? How do I get around this?

@nxnfufunezn
Copy link
Contributor

@nxnfufunezn nxnfufunezn commented Jan 24, 2016

import sys
sys.path.insert(0, "/path/to/your/package|module")

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 24, 2016

@nxnfufunezn thanks again.

This is a bit more confusing than I thought it'd be... how do I pass run_tests() the tests and options.verbose that it expects? (code as previously mentioned in gist)

Error running mach:

    ['test-webidl']

The error occurred in the implementation of the invoked mach command.

This should never occur and is likely a bug in the implementation of that
command. Consider filing a bug for this issue.

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

The details of the failure are as follows:

TypeError: run_tests() takes exactly 2 arguments (0 given)

  File "/home/kishor/projects/servo/python/servo/testing_commands.py", line 343, in test_webidl
    return run_globals["run_tests"](**kwargs)
@psdh
Copy link
Contributor Author

@psdh psdh commented Jan 24, 2016

@therealkbhat Try importing the runtests module and add a command argument for verbose output :)

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 24, 2016

Try importing the runtests module and add a command argument for verbose output :)

In runtests.py:

sys.path.insert(0, os.path.dirname(os.path.abspath(__file__)))

So I need to import runtests in testing_commands.py instead?

In testing_commands.py:

@CommandArgument('--quiet', '-q', action='store_false',
                     help="Don't print passing tests")

This is what I've added to my previous code. What am I doing wrong? I'm getting

TypeError: run_tests() got an unexpected keyword argument 'quiet'

EDIT: updated gist.

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 26, 2016

@jdm, feel free to unassign me for now.

I won't be able to work on this for a while, and I found that I need to do a bit more understanding to figure out how everything fits together! In the meantime, someone else might be able to get this fixed faster. :)

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 26, 2016

No worries, thanks for trying! :)

For future reference, it would be of great help if you push your commits to your fork and link them here, instead of putting them in gists.

@kishorbhat
Copy link
Contributor

@kishorbhat kishorbhat commented Jan 26, 2016

@wafflespeanut duly noted. :)

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

Can I take this bug if no one is working on it?

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 29, 2016

Great! Feel free to ask questions here or at the #servo channel in IRC :)

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

So here is my plan, please let me know if its OK.

  1. When run ./mach test-webidl
  2. Run update.sh once
  3. Create a virtualenv
  4. Install pip install ply in the virtualenv
  5. run `runtest.py

I can run runtest.py successfully, but when I run update.sh the auto-merge got rejected. How should I handle a failed patching? Should I still run the original WebIDL.py or just stop there and show an error?

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

Looking into the python folder, I found that we already have a requirements.txt and _virtualenv created, is there any example of other commands installing dependencies and creating virtualenv ?

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

Hi I wrote a draft version that doesn't update from mozilla-central

master...shinglyu:testwebidl

So we should download the tests/*.py from m-c in update.sh right?

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 29, 2016

@shinglyu That looks good. Go ahead and open a PR, so that people can comment on your changes :)

@nox What should we do when the patch fails?

@psdh
Copy link
Contributor Author

@psdh psdh commented Jan 29, 2016

Do we want to run the update script each time I test-webidl?

@nox
Copy link
Member

@nox nox commented Jan 29, 2016

@wafflespeanut Fix it. We store patches to be sure we don't lose stuff when updating the parser.

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 29, 2016

@psdh No, we don't!

@shinglyu I misinterpreted nox's comment. We just want to update the update.sh to include the tests folder. We should never invoke that during testing!

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

@wafflespeanut OK, I'll create a PR for the test-webidl part.

@nox For the tests folder, I don't see a clean way to download the folder from either mxr or hg.mozilla.org. I thought about reading the filenames from the local tests folder, but I fear that the mozilla-central folder may have new files or renamed files. What do you think?

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

I got an warning in the PR saying that I don't have tests. Where can I add a test for mach command?

@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Jan 29, 2016

That's just the bot shouting at any change made to script. We don't have to worry about it :)

@shinglyu
Copy link
Member

@shinglyu shinglyu commented Jan 29, 2016

@wafflespeanut Thanks!

bors-servo added a commit that referenced this issue Jan 30, 2016
Add mach test-webidl command

I updated the `WebIDL.py` from latest mozilla-central. And add a `./mach test-webidl` command. For #9397

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9459)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 30, 2016
Add mach test-webidl command

I updated the `WebIDL.py` from latest mozilla-central. And add a `./mach test-webidl` command. For #9397

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9459)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Jan 30, 2016
Add mach test-webidl command

I updated the `WebIDL.py` from latest mozilla-central. And add a `./mach test-webidl` command. For #9397

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9459)
<!-- Reviewable:end -->
@wafflespeanut
Copy link
Member

@wafflespeanut wafflespeanut commented Feb 14, 2016

fixed by #9459 and superseded by #9475

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.

None yet
7 participants
You can’t perform that action at this time.