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

[MRG] Test R minor version and add more tests #1086

Closed
wants to merge 1 commit into from

Conversation

aplamada
Copy link
Contributor

Update the R tests and add more tests to address the issues noticed in #1077 .

Feedback is welcome.

@welcome
Copy link

welcome bot commented Sep 19, 2021

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.
welcome
You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

@aplamada aplamada changed the title Test R minor version and add more tests [MRG} Test R minor version and add more tests Sep 19, 2021
@aplamada aplamada changed the title [MRG} Test R minor version and add more tests [MRG] Test R minor version and add more tests Sep 19, 2021
@aplamada
Copy link
Contributor Author

aplamada commented Jan 5, 2022

@yuvipanda When I noticed the problem with the R version (see #1077 ) I had in mind at least to systematically improve the tests for the minor version, and I thought to do it similar to the Python part.
In the meantime the master evolved and I noticed also some improvements in this direction.
What do you recommend me to do with this PR?
I understand that what I did: just improving the tests and not solving the problem is not the most constructive approach ...

@yuvipanda
Copy link
Collaborator

I didn't see this PR properly before (sorry @aplamada) and I think I've added very similar tests to #1104. Can you see if any of these tests would be helpful there? If not, let's close this PR?

I'm really sorry this wasn't reviewed sooner. I think added tests are always useful, wether they solve the underlying problem or not!

@aplamada
Copy link
Contributor Author

aplamada commented Jan 6, 2022

@yuvipanda Thanks for the reply.
The tests are very similar.
tests/conda/r could be improved similar to tests/r/ to include tests for the versions 3.6, 4.0 and 4.1.
What do you think?

I think it makes sense to close this PR.

@yuvipanda
Copy link
Collaborator

@aplamada yeah, that would be great! I haven't really paid any attention to the conda/r stuff at all. Would you be interested in working on that? I can help with more prompt review this time.

@aplamada
Copy link
Contributor Author

aplamada commented Jan 6, 2022

Yes. I am thinking to close this PR and make a new one for conda tests only.
When I will close it I will mention #1104 and the new one. What do you think?

@yuvipanda
Copy link
Collaborator

@aplamada that sounds great.

@aplamada
Copy link
Contributor Author

aplamada commented Jan 7, 2022

The PR is closed since the test are covered in #1104 and #1108 .

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.

2 participants