How and when we test?

[m7pr]

Motivation

For sure tests are painful and they slow the development, however if we ignore them for too long (I am not saying we do) we could create a problematic technical debt in the future. Tests force an author of PR to execute the code at least one time to make his/her PR reliable, but only if they are a part of the flow.

When we test?

Currently we have a thorough setup of GitHub Actions that gets triggered on each commit to a PR that is provided directly to main branch. Unfortunately, in some situations, ale like for the past couple of weeks, other branch branch become the new main branch (talking about filter_panel_refactor branch) and automated GitHub Actions tests are not trigged on recent PRs.

This can result in a PR being merged to filter_panel_refactor without any global GitHub Actions test, just to realize tests are not passing on filter_panel_refactor (that has a PR to main) seconds after the merge.

I would like to work out a procedure in which we make sure the changes were tested at least the very same way as GitHub Actions would do. Also triggering tests on GitHub Actions enables team members to see immediately that some changes are required for a PR before it is merged. Lastly it proves the verification of the tests if the author or the PR did not provide an information on which tests has been run.

I know running tests on feature branches might not always be ideal as there is a lot of other dependencies that can also change, but for some more stable moments, where other dependencies are stable, running tests on feature branches would be crucial for code stability.

I'd love to hear you opinion on the below manner. Below are my few recommendations on how to tackle that issue:

1. we enable automated GitHub Actions for PRs made to filter_panel_refactor and any future branches that became the new main for some time
2. we enable manually triggered GitHub Actions for PRs made to filter_panel_refactor and any future branches that became the new main - they are triggered manually by a click, before the author asks for the review
3. we run all the GitHub Action tests locally on machine of the PR author and we copy-paste the result of the test as an information on a PR, before the author asks for the review - this is doubtful will work on a longer run

How we test?

Is GitHub Actions the only requirement for a PR to be merged? Should we also be doing manual testing? If yes, I recommend we extend CONTRIBUTING guidelines on this repo with an information on how to run a test app, and what behavior should be verified before each PR. I would recommend creating quick videos and uploading them to a PR as a part of the testing flow. Would shinytest2 take some burden out of our plate if we incorporate them in the future?

Happy to discuss and get your view on testing

[donyunardi]

Thank you for starting this discussion @m7pr

I have previously raised this question, and I comprehend the rationale behind the current configuration, which aligns with the points you have already highlighted (i.e., avoiding development slowdown). It's possible that GHA was previously enabled on all PRs, and a decision was made to configure it in this specific manner.

However, I also understand the motivation of this discussion.

1. we enable automated GitHub Actions for PRs made to filter_panel_refactor and any future branches that became the new main for some time

I find this approach particularly intriguing and see it as a viable middle ground compared to enabling GHA for all pull requests (PRs). @cicdguy is this possible to do? If so and the team is in agreement, I am proposing that we try this for one increment and then we can reconvene for lesson learned and/or further discussion.

3. we run all the GitHub Action tests locally on machine of the PR author and we copy-paste the result of the test as an information on a PR, before the author asks for the review - this is doubtful will work on a longer run

I've done this before using a tool called act. It's definitely possible but requires a local docker setup. I agree, probably not sustainable for long run.

[m7pr]

This is doable if we extend a list of branches in builds specs, like those 2 lines

teal.slice/.github/workflows/check.yaml

Lines 11 to 12 in c8e94af

	branches:
	- main

teal.slice/.github/workflows/docs.yaml

Lines 23 to 24 in c8e94af

	branches:
	- main

should have a newline following it with - filter_panel_refactor

[cicdguy]

Yes, go ahead and do this. This is the best strategy

[pawelru]

Yes - just do this. If that helps you guys keeping code quality high and save your time with manual integration testing then go ahead with this.

[m7pr]

@donyunardi besides automated testing I'm also interested in the manual flow of testing. I'd like us to come up with few bullet points on how to run and record manual tests for reviewers.

[cicdguy]

Also consider using pre-commit hooks, which are available in this repo.

[pawelru]

I don't quite get what you mean by "manual testing". Generally speaking, I would rather keep manual testing as minimal as possible and move as much as possible into auto-tests for obvious reasons. I hope that we all agree on that :)
If you are asking about what should be included in typical code review process from devs side then my recommendation is to still keep manual checks (deliberately not using "tests" term) alongside code checks to cover things like UX look&feel, responsiveness, etc. that are difficult (if not impossible) to include in auto-tests. Once done - there shouldn't be a need to "recheck" it again.

[m7pr]

Yes @pawelru by manual testing I meant manual testing of the FE (frontend) - the stuff that we have in tests/testthat is mostly unit tests for the backend. So as you mentioned I am referring to UX, responsiveness and etc. I also have as much manual testing incorporated in some kind of automation, but for now I realize we do start teal.gallery app and we look manually on the overall behavior and appearance. but trying to figure out best practices for manual testing

[pawelru]

for now I realize we do start teal.gallery app and we look manually on the overall behavior and appearance

I just chatted about this with @donyunardi yesterday. From my perspective it is somewhere in top priorities for the nearest future - I mean to convert those manual checks into auto-tests as much as possible. I was thinking where to place it and right now I think those should live mostly in teal.modules.xyz plus teal (with its family) - in particular: not in teal.gallery. But this is a topic for another discussion.

[m7pr]

Thanks for the discussion @pawelru . One more thing, if we ever get to automated tests of frontend with the help of shinytest2 then we should have test-cases outlined in writing, so that we know how to cover tests with functions. This means we need written test-cases for manual testing first. And if we have any right now, it would be great to have them in one place

[kartikeyakirar]

@m7pr I have a few suggestions for manual testing. here are my thoughts

Test Plan:(for reviewer)
(In most of the PR sample app is given with the following points might speed up process and will also cover edgecases)

• Define the objectives and scope of the testing.
• Outline the areas, features, and functionalities to be tested. Specify any specific testing requirements or constraints

Test Environment Setup: (this vary helpful write some additional pieces to setup correct environment , specially for newcomers)

• Provide instructions on setting up the test environment, including software versions, configurations, and dependencies.
• Specify any test data or sample inputs required for the testing.

Test Scenarios:

• Create a checklist or a list of bullet points for the specific test scenarios to be executed.

Video Demonstrations: (I really liked idea of small demo presentation of the developer testing the feature and bug)

• Record videos demonstrating the execution of critical test scenarios.
• Show the expected behavior and compare it with the actual behavior observed during testing.

Expected Results:

• Document the expected outcomes or behaviors for each test scenario.
• Include specific outputs, UI changes, or functional responses that should be observed.

Edge Case Coverage:

• Developer and Review should add edge cases in a document which are being tested.
• it will create a good list of checks every time a new modification is made.

Shinytest2 Integration:

• Consider the possibility of incorporating Shinytest2, a testing framework for Shiny apps, to automate UI testing.
• We can start evalusting if shinytest2 align with current setup and considering integration for future. but sure we could start small

[m7pr]

added a PR to extend builds also for filter_panel_refactor #318

[chlebowa]

There are two requirements before merging: checks and at least one approval. Well, three, the third being no change requests, thought the latter can be waived. The point is there is a responsibility placed on the reviewer.

By custom, when we put up a PR we describe the intended behavior and the actions that we explicitly wish reviewers to test on their machines, but we also count on the reviewer to try to creatively break the app in other ways.
Providing a light-weight but exhausting testing app is good form if example apps in the help files or something like basic-teal do not allow to observe the new behavior in much detail. Examples provided by @gogonzo in #165 are excellent... examples.
Accompanying branches from other repos should obviously be clearly listed in the PR description, although if we keep the branch naming convention, staged.dependencies should create a suitable environment. Some people may wish to have a feature branch installed if their other work depends on main, but they can have a feature branch checked out and add a devtools::load_all(<that_package>) for the purpose of testing.

I understand there have been attempts at introducing shinytest2 but they were abandoned as it couldn't run on CI/CD pipelines. Supposedly something about the package was changed but we haven't tested it so far.

[m7pr]

Thanks everyone for the input! @pawelru @chlebowa what do you think about creating a task/github_issue where we outline manual test cases for the basic-teal app?

So that we have at least one app, with few statements like:

• global disable button disables all filters besides locker filters
• remove all filters for a dataset button removes all filters besides locked filters
• it is possible to have manual or ranged input for numerical variables for non-disabled filters
• categorical filter for X variable in Y dataset has Z options to be selected

and many more. If we get an exhausted list of 20-30 test cases it is easier for testers and future users to ensure nothing is omitted. Also this list can be reshaped later into automated tests with shinytest2! But to know what we want to test with shinytest2 it would be great to have a list of assumptions to be tested first. Lastly this list should be reviewed if it's up to date during every PR - I can imagine some assumptions gets removed during breaking changes. This should be updated the same as regular backend tests are updated. Just wanted to get your thoughts on it.

[pawelru]

This all makes a lot of sense to me but let me recall my thought from one of the posts above:

I was thinking where to place it and right now I think those should live mostly in teal.modules.xyz plus teal (with its family) - in particular: not in teal.gallery. But this is a topic for another discussion.

Everything what you outlined (I know those are some random examples but still) should live where tested functionality lives - here teal.slice, in particular: not a basic-teal app. In fact, there is very few tests that should live on the app level and those should be more of a integration test assuming that each functionalities are tested elsewhere. That would address the issue of processing breaking changes in many repos which is a very painful experience.
Therefore, this (i.e. test cases definition) should be an integral part of development activities. I don't mind doing this in a separate GH issue but from the big perspective this should belong to the one epic.

[m7pr]

@pawelru I don't quite follow yet how the process and roadmaps here are executed, so I defer to you when it comes to putting that in action. I agree that it's fine to test modules for teal.modules.xyz, but I wanted to discuss how we test filter-panel and only filter-panel no matter what modules are used in the teal app. The scenarios for manual tests that I outlined above were only teal.slice specific. And they are actual scenarios for teal.slice's filter-panel - not random ideas.

Therefore, this (i.e. test cases definition) should be an integral part of development activities.

I totally agree. We are on the same page. I want to test the package during the development. But with no scenarios for manual-testing it is hard to test the package besides running

devtools::test()
devtools::check()
lintr::lint_package()

+ running a random app and then trying to break everything that is in there.
It's hard to eliminate every possible break if we didn't outline scenarios.

I don't want to test teal.modules.xyz during tests on teal.slice. I am focusing on the filter-panel. Does that make more sense?

[m7pr]

I think it's my mistake that I assumed we are talking only about teal.slice and I think, @pawelru, your approach was more general. Sorry for the confusion

[pawelru]

No worries. I think that you guys should create a list of possible test cases for refactored filter panel as we are already reach some maturity there with regards to the scope and we are already beyond the phase where things can drastically change. My recommendation would be to include also implementation and don't do one task for definition and second for implementation.
Let me summon @gogonzo (in the absense of @donyunardi; fyi @lcd2yyz) - is this something we can make (and include) before the planning?

[m7pr]

Amazing. I've seen a task/epic/milestone for including shinytest2. Creating manual-tests aka scenarios could be a first part of it

[m7pr]

For the shinytest2 use cases I followed

• Using shinytest2 with continuous integration
• Getting started with shinytest2
  and setup GHA builds for this repository and it looks like it run shinytest2 (here is the job).

@cicdguy do you remember if you run shinytest2 before in GHA builds and what was the issue that caused we could not set it up?

[cicdguy]

No issues running shinytest2. We already do this for another package. We need to set the environment variable NOT_CRAN=true for it to really work though, since CRAN doesn't have Chromium on its servers.

[m7pr]

glad to hear. cc @chlebowa to be aware of it. @cicdguy can you point to a package where you use shinytest2? just out of curiosity?

[cicdguy]

https://github.com/insightsengineering/teal.modules.hermes has shinytest2 tests.

Remember to set this in the check workflow if you want to enable it: https://github.com/insightsengineering/teal.modules.hermes/blob/main/.github/workflows/check.yaml#L30

[m7pr]

Amazing, thanks cicdguy

[m7pr]

@cicdguy since we've got you here - would it be possible to setup github commands (like this on tidyverse/dplyr) so that if someone types e.g. /run_all (this is the command) we get a version of Check build but it runs on more than one docker container? The purpose of this command would allow us to have typical Check builds on one environment (e.g. R 4.2.2 Bioc 3.16) but if we want to merge a huge change then we are allowed to run tests on multiple containers, hence multiple R versions on a feature branch.

[cicdguy]

We have internal tests that do this already.

Also the issue "commands" are spammy based on feedback we've received in the past, so we don't do that anymore.

[m7pr]

got it @cicdguy thanks. Can you shred some more light on the internal tests on multiple R versions? Does it happen during the release?

[cicdguy]

Tagged you in the related notifications @m7pr

[m7pr]

Hey all, to summarize the discussion

• we agreed to run automated tests on feature branches that became new main branches for a while
• we discussed manual tests and agreed on other talks to prepare them when we are done with the stable version
• once we have manual tests it will be easier to create shinytest2 tests