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

improved testing/formatting section #36576

Closed
wants to merge 6 commits into from

Conversation

alexanderLinear
Copy link

@alexanderLinear alexanderLinear commented Mar 23, 2023

This pull request is purely about documentation improvements.
It first re-orders and slightly improves formatting so that the overall meaning and usability of the referenced tooling is eased.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for contributing the extra details. One small fixup requested though.

CONTRIBUTING.md Outdated

```bash
python3 -m pip install -r test/requirements.txt
```

To run the tests run ``nosetests`` in the root of the repository.
These tests require several dependencies that can be installed either from the ROS repositories or via pip(list built based on the content of [.travis.yaml](https://github.com/ros/rosdistro/blob/master/.travis.yml):
The following modules/packages are needed and will see install by avobe comment.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The following modules/packages are needed and will see install by avobe comment.
The following modules/packages are needed and will see install by above comment.

Copy link
Author

Choose a reason for hiding this comment

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

thanks, used it with a bit more of needed change.

@tfoote
Copy link
Member

tfoote commented Mar 23, 2023

Also please replace the body of the issue/PR with a real description to enable easier review for others in the future.

@alexanderLinear
Copy link
Author

Also please replace the body of the issue/PR with a real description to enable easier review for others in the future.

done.

Copy link
Member

@tfoote tfoote left a comment

Choose a reason for hiding this comment

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

Thanks for the attention. @quarkytale Can you try this out and if it works for you merge?


```bash
python3 -m pip install -r test/requirements.txt
```

To run the tests run ``nosetests`` in the root of the repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

This line made sense to me after installing dependencies, and after the list of installed modules.

If you want to run the tests before submitting, first install the dependencies. Using `pip` is recommended.
#### Testing

To run the tests run ``nosetests`` (nose tests) in the root of the repository.
Copy link
Contributor

Choose a reason for hiding this comment

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

A separate section is a good idea but maybe some other intro line or about nose https://nose.readthedocs.io/en/latest/index.html? It would be better for uses to have the command after installing dependencies.


To run the tests run ``nosetests`` (nose tests) in the root of the repository.

Note: There's a [known issue](https://github.com/disqus/nose-unittest/issues/2) discovered [here](https://github.com/ros/rosdistro/issues/16336) that most tests won't run if you have the python package `nose-unitttest` installed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Note: There's a [known issue](https://github.com/disqus/nose-unittest/issues/2) discovered [here](https://github.com/ros/rosdistro/issues/16336) that most tests won't run if you have the python package `nose-unitttest` installed.
Note: There's a [known issue](https://github.com/disqus/nose-unittest/issues/2) discovered [here](https://github.com/ros/rosdistro/issues/16336) that most tests won't run if you have the python package `nose-unittest` installed.


```bash
python3 -m pip install -r test/requirements.txt
```

To run the tests run ``nosetests`` in the root of the repository.
These tests require several dependencies that can be installed either from the ROS repositories or via pip(list built based on the content of [.travis.yaml](https://github.com/ros/rosdistro/blob/master/.travis.yml):
The following modules/packages are needed and will see install by the above command.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The following modules/packages are needed and will see install by the above command.
The following modules/packages are needed and will be installed by the above command.


Note: There's a [known issue](https://github.com/disqus/nose-unittest/issues/2) discovered [here](https://github.com/ros/rosdistro/issues/16336) that most tests won't run if you have the python package `nose-unitttest` installed.
There is further the tool ``rosdistro_reformat`` which will fix most formatting errors such as alphabetization and correct formatting. This tool takes an ``index`` parameter that is a required command line argument. With option ``-n`` a dry run is possible.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
There is further the tool ``rosdistro_reformat`` which will fix most formatting errors such as alphabetization and correct formatting. This tool takes an ``index`` parameter that is a required command line argument. With option ``-n`` a dry run is possible.
There is a tool ``rosdistro_reformat`` which will fix most formatting errors such as alphabetization and correct formatting. This tool takes an ``index`` parameter that is a required command line argument. With option ``-n`` a dry run is possible.

@quarkytale quarkytale added the changes requested Maintainers have asked for changes to the pull request label Mar 27, 2023
@quarkytale
Copy link
Contributor

quarkytale commented Mar 28, 2023

Also might be worth noting nose isn't compatible with >= Python3.10 nose-devs/nose#1118.
And we do have plans removing it #36211.

@quarkytale
Copy link
Contributor

Hi @AlexanderStohr would you like to address some of my previous comments before I give it another review

@nuclearsandwich
Copy link
Member

@alexanderLinear are you still interested in addressing this feedback and updating this PR?
If so, great! If not, could you please close it.

@alexanderLinear
Copy link
Author

@alexanderLinear are you still interested in addressing this feedback and updating this PR? If so, great! If not, could you please close it.

sorry, was busy in recent times.
approaching a long weekend with only few other dates. will try to go on with it again then.

@github-actions
Copy link

This PR hasn't been activity in 14 days. If you are still are interested in getting it merged please provide an update. Otherwise it will likely be closed by a rosdistro maintainer following our contributing policy. It's been labeled "stale" for visibility to the maintainers. If this label isn't appropriate, you can ask a maintainer to remove the label and add the 'persistent' label.

@github-actions github-actions bot added the stale Issue/PR hasn't been active in a while and may be closed. label Jul 14, 2023
@emersonknapp
Copy link
Contributor

Closing as stale per guidelines. If you still wish to contribute this change, feel free to open a new PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes requested Maintainers have asked for changes to the pull request stale Issue/PR hasn't been active in a while and may be closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants