Skip to content

[INFRA] Adding tests #117

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.

Sign up for GitHub

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

Jump to bottom
Merged
csouchet merged 3 commits into from
Oct 20, 2022
Merged

[INFRA] Adding tests #117

csouchet merged 3 commits into from
Oct 20, 2022

Conversation

@ColinFay
Copy link
Contributor

@ColinFay ColinFay commented Oct 12, 2022

This closes #6

Funs has been moved to another file, so that they can be tested independently.

  • devtools::check()

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 28 ]

  • covr::package_coverage()

bpmnVisualization Coverage: 100.00%
R/bpmnVisualization.R: 100.00%
R/funs.R: 100.00%

@ColinFay
chore: Adding tests
745cf1f 
- devtools::check()

[ FAIL 0 | WARN 0 | SKIP 0 | PASS 28 ]

- covr::package_coverage()

bpmnVisualization Coverage: 100.00%
R/bpmnVisualization.R: 100.00%
R/funs.R: 100.00%
@CLAassistant
Copy link

CLAassistant commented Oct 12, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@tbouffard tbouffard requested a review from csouchet October 12, 2022 13:49
@tbouffard tbouffard added the chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) label Oct 12, 2022
@tbouffard
Copy link
Member

tbouffard commented Oct 12, 2022 •
edited
Loading

Thanks for your contribution, we are going to review it in the next days.
In the meantime, I have seen that the GitHub Actions Checks are failing. Can you have a look at it? If you suspect that there is an issue in the workflow definition not related to your changes, please let us know.

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 12, 2022

Sure thing.

Yes, that's an issue with {htmlwidgets} that depends on {shiny} but doesn't actually import it, yet it still needs it in its renderer (which seems to be a bug in {htmlwidget}?).
https://cran.r-project.org/web/packages/htmlwidgets/index.html

Anyway, feel like the issue was there from before my tests (package might not be working if you only had {htmlwidgets} installed), but I'll add the dep in the widget builder so that it works.

What do you prefer :

  1. Hard requirement, i.e. direct dependency
  2. Checking if the package is installed with rlang::is_installed() and then adding {shiny} as Suggest

?

@tbouffard tbouffard mentioned this pull request Oct 12, 2022
Open
@tbouffard
Copy link
Member

tbouffard commented Oct 14, 2022

Thanks for the details about htmlwidgets.
That's strange we didn't notice it before but maybe because shiny was installed on the machine we used for the test 😄

Anyway, tests are proving the issue, so let's fix this. I would prefer solution 2 because our code doesn't depend on shiny directly. We add shiny only because of the presumed bug in htmlwidgets.

@tbouffard tbouffard added the hacktoberfest-accepted Accepted Pull Request during Hacktoberfest label Oct 18, 2022
@ColinFay
fix: Ensuring shiny is installed
10bca17 
Also, added {rlang} as dep and {shiny} as suggest
@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 19, 2022

Hey,

I just updated the code to reflect these changes :)

Cheers,
Colin

@tbouffard tbouffard changed the title chore: Adding tests [INFRA] Adding tests Oct 20, 2022
tbouffard
tbouffard approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@tbouffard tbouffard left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot @ColinFay I really appreciate this contribution: it detects and fix an issue with the missing {shiny} dependency, it improves the documentation of the functions and the function extraction makes the code easier to read 💯

As mentioned in #33 (comment), this PR includes additional formatting of the code, but this is perfectly acceptable as the project currently has no formatting guidelines.
This is definitely something we will have to work on soon!

csouchet
csouchet approved these changes Oct 20, 2022
View reviewed changes
Copy link
Contributor

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution and for your help on this part 🙏🏻

I will be happy if you want to help us on another subject 🙂

@csouchet csouchet merged commit 33699f4 into process-analytics:main Oct 20, 2022
This was referenced Oct 22, 2022
Merged
Open
@tbouffard
Copy link
Member

tbouffard commented Oct 27, 2022

Hi @ColinFay and thanks again for your contributions.

❓ I have one last question because I am curious 😸 .
Apparently, you made a contribution as part of Hacktoberfest. I would like to know how you found out about our repository.
This would help us in the future to reach more potential contributors.

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

Reviewers

@tbouffard tbouffard tbouffard approved these changes

+1 more reviewer

@csouchet csouchet csouchet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

chore Build, CI/CD or repository tasks (issues/PR maintenance, environments, ...) hacktoberfest-accepted Accepted Pull Request during Hacktoberfest

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[TEST] Add automatic tests

4 participants

@ColinFay @CLAassistant @tbouffard @csouchet