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 of relevance on calculates #704

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

Conversation

lognaturel
Copy link
Member

This is a pattern that we broke in Enketo a while ago. I want to make sure it's explicitly tested here because it is a bit surprising.

@lognaturel lognaturel requested a review from seadowg January 6, 2023 22:44
@@ -530,6 +530,33 @@ public void non_relevant_node_values_are_always_null_regardless_of_their_actual_
assertThat(scenario.answerOf("/data/some-field"), is(intAnswer(42)));
}

// Users use relevance on calculates to ensure that calculations are only run when the values
Copy link
Member

Choose a reason for hiding this comment

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

Is it important to make sure that the calculation isn't executed or just that the calculation isn't "answered"? I'm guessing it's the latter (as that's what this test is about), but the comment suggests it might the former.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great and deep question. Now I'm no longer 100% sure of the intent. This led me to some strange docs: getodk/docs#1547 I will come back to it.

@lognaturel lognaturel marked this pull request as draft January 20, 2023 00:07
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