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 test for language slug in metadata feature #1637

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

minhaminha
Copy link
Contributor

Technical Summary

This is a followup PR to this one, which allows for the current screen's language slug to be accessible through the session metadata. This just adds a test to cover that area.

Safety Assurance

Safety story

This only adds a test and test-specific code blocks to existing functions. evaluateXpath is currently written to initialize the session locales every time it's called (at least test-specific scenarios) which makes it necessary to manually globalLocalizer's available and default locales, because the test is looking to verify that that value can be accessed through the applanguage xpath,

Automated test coverage

The whole PR is the test coverage.

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.

@minhaminha minhaminha requested a review from shubham1g5 November 1, 2024 23:11
@minhaminha
Copy link
Contributor Author

HI @shubham1g5, apologies for the delay in getting this up! I made the mistake of trying to model it after the WindowWidth test which led me astray. I'm still not sure about how valid this test is given that it feels a bit heavy handed (having to manually set the GlobalLocalizer's available locales and current locale). I wrote it in the description too but this does indeed test the ability to access the localizer's locale through that specific xpath which I think is enough but wanted to get your thoughts on it.

Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 70.29%. Comparing base (af51f19) to head (bb06780).

Files with missing lines Patch % Lines
...are/formplayer/application/DebuggerController.java 80.00% 0 Missing and 1 partial ⚠️
...are/formplayer/application/FormSessionFactory.java 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1637      +/-   ##
============================================
+ Coverage     70.22%   70.29%   +0.07%     
- Complexity     2003     2008       +5     
============================================
  Files           254      254              
  Lines          7899     7911      +12     
  Branches        742      744       +2     
============================================
+ Hits           5547     5561      +14     
+ Misses         2070     2067       -3     
- Partials        282      283       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

@minhaminha I think the way we want to write these tests is by using the changeLanguage calls like here. We should not really need to make a change in any bean models. The big difference between windowWidth and locale here is that HQ conveys FP the windowWitdth in the request payload while locale is already stored and maintained on FP side with HQ only needing to convey the local changes on browser side by using the /change_locale endpoint on FP. Let me know if I should provide any more details here and thanks for following up with tests!

@minhaminha
Copy link
Contributor Author

@shubham1g5 all the bean changes are so that evaluateXpathRequest can be simulated properly. I've repeatedly tried to make use of changeLanguage but calling evaluateXpath in the test context forces a re-initialization of the session, which resets the available locales back to 'default' and 'en' and sets the global locale back to 'default'. Do you have any other ideas of how to preserve the specified locale and while also getting the xpath value?

@shubham1g5
Copy link
Contributor

shubham1g5 commented Nov 6, 2024

@minhaminha Is there an app I can see this feature working ? I want to cross check the behaviour you are seeing in the tests with the actual feature behaviour.

Update: That I tried this on my end by displaying the value of the session variable here but when I go the form and change the language to any other language, this doesn't really update the the new changed language both in the label display and when using the evaluate Xpath to calculate value of instance('commcaresession')/session/context/applanguage

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