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 assertions tests #1411

Merged
merged 10 commits into from
Jul 7, 2023
Merged

add assertions tests #1411

merged 10 commits into from
Jul 7, 2023

Conversation

Robert-Costello
Copy link
Contributor

Technical Summary

relates to dimagi/commcare-core#1267

Safety Assurance

just tests

Safety story

Automated test coverage

is only tests

QA Plan

relates to this QA ticket: https://dimagi-dev.atlassian.net/browse/QA-5258

Special deploy instructions

  • This PR can be deployed after merge with no further considerations.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations.

Review

  • The set of people pinged as reviewers is appropriate for the level of risk of the change.

Comment on lines 86 to 87
SessionNavigationBean bean = request.getNavigationBean(selections);
return request.requestWithBean(bean);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you can simplify this a bit:

Suggested change
SessionNavigationBean bean = request.getNavigationBean(selections);
return request.requestWithBean(bean);
return request.request(selections);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

simplified here cd051b4

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

Hey Robert, the PR looks good. Wondering if it's possible to comment on the PR the changes in .ccz file for the sake of visibility.

Also we should add tests for a few different type of edge cases we discovered during testing this -

  1. Assertions that use external instances instance(x)
  2. Assertions on root menu with instance connectors defined in menu

It would also be great to have both positive and negative test cases around assertions. For eg we can write tests testing count(instance('commcaresession')/session/user/data/project_role) > 0 for 2 different users (aka with 2 different restores) one with project_role and other without and check assertions accordingly.

CommandListResponseBean.class);
assertNotNull(response0);
// Check failing assertion
assertThrows(Exception.class, () -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we also verify that exception has the right message as per assertions here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

verified here 19a982c

@Robert-Costello
Copy link
Contributor Author

Robert-Costello commented Jun 27, 2023

Wondering if it's possible to comment on the PR the changes in .ccz file for the sake of visibility.

in the suite file I added assertions and instance references to the groups fixture already being used.

menu 0:
<menu id="m0">
    <text>
         <locale id="modules.m0"/>
     </text>
    <instance id="groups" src="jr://fixture/user-groups"/>
    <assertions>
         <assert test="count(instance('groups')/groups/group) = 1">
            <text>
                 <locale id="case_sharing.exactly_one_group"/>
             </text>
         </assert>
    </assertions>
    <command id="m0-f0"/>...
</menu>

menu 1:
<menu id="m1">
    <text>
        <locale id="modules.m1"/>
    </text>
    <instance id="groups" src="jr://fixture/user-groups"/>
    <assertions>
        <assert test="count(instance('groups')/groups/group) = 2">
              <text>
                 <locale id="case_sharing.exactly_one_group"/>
             </text>
         </assert>
     </assertions>
     <command id="m1-f0"/>...
</menu>

In addition I also added a new menu m19 that is just a duplicate of m18 that is setup with an assertion that only allows access to users with project_role set to "admin".

@Robert-Costello
Copy link
Contributor Author

Robert-Costello commented Jul 5, 2023

Several tests that are failing here are passing locally. They all look related to the suite file changes in basic test app.
@shubham1g5 any idea why that might be?

@shubham1g5
Copy link
Contributor

They seem to fail locally as well for me. You probably wanna cross-check changes in basic.ccz.

@Robert-Costello
Copy link
Contributor Author

It turns out that it's the submodule ref that is causing the failures. When I switch to the formplayer branch locally, I get the same failures.

@Robert-Costello Robert-Costello mentioned this pull request Jul 6, 2023
2 tasks
@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #1411 (0663863) into master (74b4e0f) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #1411      +/-   ##
============================================
- Coverage     69.66%   69.61%   -0.06%     
+ Complexity     1918     1916       -2     
============================================
  Files           247      247              
  Lines          7529     7529              
  Branches        674      674              
============================================
- Hits           5245     5241       -4     
- Misses         2020     2026       +6     
+ Partials        264      262       -2     

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

<Sync xmlns="http://commcarehq.org/sync">
<restore_id>9f7c4e0a3a8ca4e2ecf54444fb7e70cf</restore_id>
</Sync>
<Registration xmlns="http://openrosa.org/user/registration">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we trim the file to only contain what's necessary for the test. I believe you should be able to remove most of the info in this file for the assertion test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. Yes, I'll trim it down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

restore updated here 0663863

Copy link
Contributor

@shubham1g5 shubham1g5 left a comment

Choose a reason for hiding this comment

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

💯

@Robert-Costello Robert-Costello merged commit abc3587 into master Jul 7, 2023
3 of 4 checks passed
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.

3 participants