Conditional set output is wrong #6244

scottbell · 2023-02-01T12:26:55Z

Closes #5918

Describe your changes:

Removed default output from being used before component has received and evaluated valid telemetry.
Added check in staleness for existence of staleness response (to prevent thrown error)

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

…t-output-is-wrong

scottbell · 2023-02-01T12:29:05Z

Here's the new behavior:

Screen.Recording.2023-02-01.at.1.22.18.PM.mov

I'm turning of YAMCS data to simulate the "no data" situation, then starting YAMCS to show the ConditionSet receiving data. It also works if you create a Sine Wave Generator with a loading delay.

If this looks good, I'll add an e2e test with a Sine Wave Generator that has a loading delay.

codecov · 2023-02-01T12:36:54Z

Codecov Report

Merging #6244 (3dab182) into master (eff0cc9) will increase coverage by 1.58%.
The diff coverage is 22.22%.

❗ Current head 3dab182 differs from pull request most recent head 1d1b9af. Consider uploading reports for the commit 1d1b9af to get more accurate results

@@            Coverage Diff             @@
##           master    #6244      +/-   ##
==========================================
+ Coverage   53.33%   54.91%   +1.58%     
==========================================
  Files         626      626              
  Lines       26617    26617              
  Branches     2403     2403              
==========================================
+ Hits        14195    14616     +421     
+ Misses      11758    11341     -417     
+ Partials      664      660       -4

Flag	Coverage Δ		*Carryforward flag
e2e-ci	`39.35% <ø> (ø)`		Carriedforward from 7ec9dc7
e2e-full	`51.08% <ø> (ø)`		Carriedforward from 7ec9dc7
e2e-stable	`55.28% <50.00%> (+17.10%)`	⬆️
unit	`49.31% <22.22%> (+0.11%)`	⬆️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
src/plugins/condition/components/Condition.vue	`0.00% <ø> (ø)`
...ugins/condition/components/ConditionCollection.vue	`0.00% <0.00%> (ø)`
src/plugins/condition/components/ConditionSet.vue	`0.00% <ø> (ø)`
src/plugins/condition/components/Criterion.vue	`0.00% <ø> (ø)`
...ns/displayLayout/components/AlphanumericFormat.vue	`0.00% <0.00%> (ø)`
...plugins/displayLayout/components/TelemetryView.vue	`2.60% <0.00%> (ø)`
src/plugins/condition/ConditionManager.js	`62.37% <50.00%> (-0.27%)`	⬇️

... and 71 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eff0cc9...1d1b9af. Read the comment docs.

unlikelyzero · 2023-02-01T18:47:10Z

https://www.w3.org/WAI/ARIA/apg/patterns/accordion/ is a good guideline for the rules within the condition set.

Mainly isExpanded and button

…a/openmct into 5918-conditional-set-output-is-wrong

shefalijoshi · 2023-02-06T22:06:42Z

@scottbell @charlesh88 If you drag the condition set with output --- into a display layout, it displays a different output. Expectation is that it should also show --- right?

ConditionSetDefaultOutput.mov

scottbell · 2023-02-06T22:25:10Z

@shefalijoshi

Be sure the Sine Wave Generators you're testing with have a somewhat low frequency and substantial delay. Also, changing the delay of a Sine Wave Generator necessitates a page reload (from what I can tell):

Screen.Recording.2023-02-06.at.11.23.10.PM.mov

scottbell · 2023-02-07T13:28:01Z

@shefalijoshi @charlesh88 Regarding the difference between DisplayLayouts and the ConditionSet view on historical data, this appears to be a pre-existing issue in master from what I can tell:

Screen.Recording.2023-02-07.at.2.25.27.PM.mov

Namely, the ConditionSet view only updates if it's Local Clock mode:

Screen.Recording.2023-02-07.at.2.26.50.PM.mov

I've got something cobbled together to fix the historical data issue here.

From what I can tell, the "Test Data" also only seems to work in Local Clock mode. Is that expected?

Screen.Recording.2023-02-07.at.3.55.27.PM.mov

akhenry · 2023-03-15T21:38:02Z

In testing this I found a weird edge-case, which I will file a followup for because I think it's actually a long-standing issue with conditional styling and not an issue with condition sets themselves.

Drag a condition set with no flowing telemetry (as created here) into a display layout
Define some conditional style rules
Save
Note that whichever conditional style was last selected remains active in the display layout. Navigating away clears it.

akhenry · 2023-03-15T21:48:00Z

Filed #6444 for the followup issue

src/plugins/condition/components/ConditionCollection.vue

…t-output-is-wrong

charlesh88

I'm now seeing composition problems. Specifically:

Make a new Condition Set (CS).
Add telem to it (sine wave gen). Define condition(s).
Save and exit editing.
In the tree, expand the CS.
Use the "Remove" action to remove the telem element. The element will appear to be removed. The CS outputs "---" but incorrectly highlights the Default condition.
Collapse the CS in the tree, then re-expand it. Note that the telem element returns, and is clickable/navigable. The CS is not evaluating, and displays "---".
Edit the CS, note that the telem element appears in the CS's Elements pool and the CS does not evaluate it.

I tested this with another domain object (Gauge) and did not see the same issue. No console errors were thrown during the above sequence.

Default condition hilited after telem element removed

Telem element reappears after toggle in tree

scottbell · 2023-03-16T17:53:51Z

@charlesh88 I think the composition issue is separate from this PR, in that the behavior is also happening in master. It may be due to this recent PR? Here's a screenshare of it on master:

Screen.Recording.2023-03-16.at.6.51.17.PM.mov

scottbell · 2023-03-16T18:04:30Z

@charlesh88 I think the composition issue is separate from this PR, in that the behavior is also happening in master. It may be due to this recent PR? Here's a screenshare of it on master:

Tested before that other recent PR in, and it looks like this bug predates that too.

scottbell · 2023-03-16T18:20:02Z

@charlesh88 I think the composition issue is separate from this PR, in that the behavior is also happening in master. It may be due to this recent PR? Here's a screenshare of it on master:

Tested before that other recent PR in, and it looks like this bug predates that too.

@charlesh88 talked to @akhenry about this, and we've filed a separate issue for the tree composition problem. Regarding this:

The CS outputs "---" but incorrectly highlights the Default condition.

I'll look into it for this PR.

scottbell · 2023-03-17T12:30:10Z

@charlesh88 The current condition should be getting cleared now when there isn't telemetry:

Screen.Recording.2023-03-17.at.1.26.16.PM.mov

Out of office

scottbell added 4 commits January 27, 2023 22:26

debug code

a18bf4e

Merge remote-tracking branch 'origin/master' into 5918-conditional-se…

c9988f5

…t-output-is-wrong

Merge remote-tracking branch 'origin/master' into 5918-conditional-se…

93f9c67

…t-output-is-wrong

only use default if we've evaluated as default

ac8f4db

scottbell linked an issue Feb 1, 2023 that may be closed by this pull request

Conditional Set "output" is wrong #5918

Closed

7 tasks

scottbell requested review from shefalijoshi and charlesh88 February 1, 2023 12:29

revert

239548c

Merge branch 'master' into 5918-conditional-set-output-is-wrong

ae864d0

shefalijoshi mentioned this pull request Feb 1, 2023

Conditional styling doesn't apply Default styles when no data is present #5925

Open

7 tasks

ozyx added this to the Target:2.1.6 milestone Feb 1, 2023

Merge branch 'master' into 5918-conditional-set-output-is-wrong

3e60fc1

scottbell self-assigned this Feb 2, 2023

scottbell added 7 commits February 3, 2023 18:33

Merge branch 'master' into 5918-conditional-set-output-is-wrong

24e7371

Merge branch 'master' into 5918-conditional-set-output-is-wrong

2792609

wip

e3bdd00

Merge branch 'master' into 5918-conditional-set-output-is-wrong

ecc608b

lint

2bfa142

Merge branch '5918-conditional-set-output-is-wrong' of github.com:nas…

336f53f

…a/openmct into 5918-conditional-set-output-is-wrong

add e2e test for conditional sets

395ddc9

resolve conflicts

a10d525

scottbell added 2 commits February 8, 2023 14:53

Merge branch 'master' into 5918-conditional-set-output-is-wrong

8633346

Merge branch 'master' into 5918-conditional-set-output-is-wrong

844f6c3

akhenry removed this from the Target:2.1.6 milestone Feb 9, 2023

scottbell and others added 7 commits March 14, 2023 11:13

remove debug code

0c11199

remove debug code

2a36698

we need to set these urls for the tests to function

1aa7278

we need to set these urls for the tests to function

93e46fe

Merge branch 'master' into 5918-conditional-set-output-is-wrong

ca072e3

Merge branch 'master' into 5918-conditional-set-output-is-wrong

d8cc7bd

Merge branch 'master' into 5918-conditional-set-output-is-wrong

f0b3e06

akhenry approved these changes Mar 15, 2023

View reviewed changes

src/plugins/condition/components/ConditionCollection.vue Outdated Show resolved Hide resolved

Merge branch 'master' into 5918-conditional-set-output-is-wrong

e15d431

akhenry enabled auto-merge (squash) March 15, 2023 21:49

scottbell added 2 commits March 16, 2023 09:44

fix function name

b77d9a2

Merge remote-tracking branch 'origin/master' into 5918-conditional-se…

c784024

…t-output-is-wrong

charlesh88 previously requested changes Mar 16, 2023

View reviewed changes

Merge branch 'master' into 5918-conditional-set-output-is-wrong

4a5fa07

scottbell added 3 commits March 16, 2023 19:20

Merge branch 'master' into 5918-conditional-set-output-is-wrong

ee90efd

Merge branch 'master' into 5918-conditional-set-output-is-wrong

7ec9dc7

clear current condition

3dab182

scottbell requested review from charlesh88 and removed request for 360macky March 17, 2023 12:30

Merge branch 'master' into 5918-conditional-set-output-is-wrong

1d1b9af

akhenry added the sim-3 label Mar 17, 2023

akhenry merged commit 6cb5c47 into master Mar 17, 2023

akhenry deleted the 5918-conditional-set-output-is-wrong branch March 17, 2023 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conditional set output is wrong #6244

Conditional set output is wrong #6244

scottbell commented Feb 1, 2023 •

edited by akhenry

scottbell commented Feb 1, 2023 •

edited

codecov bot commented Feb 1, 2023 •

edited

unlikelyzero commented Feb 1, 2023

shefalijoshi commented Feb 6, 2023

scottbell commented Feb 6, 2023 •

edited

scottbell commented Feb 7, 2023 •

edited

akhenry commented Mar 15, 2023

akhenry commented Mar 15, 2023

charlesh88 left a comment

scottbell commented Mar 16, 2023

scottbell commented Mar 16, 2023

scottbell commented Mar 16, 2023 •

edited

scottbell commented Mar 17, 2023

Conditional set output is wrong #6244

Conditional set output is wrong #6244

Conversation

scottbell commented Feb 1, 2023 • edited by akhenry

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

scottbell commented Feb 1, 2023 • edited

codecov bot commented Feb 1, 2023 • edited

Codecov Report

unlikelyzero commented Feb 1, 2023

shefalijoshi commented Feb 6, 2023

scottbell commented Feb 6, 2023 • edited

scottbell commented Feb 7, 2023 • edited

akhenry commented Mar 15, 2023

akhenry commented Mar 15, 2023

charlesh88 left a comment

Choose a reason for hiding this comment

scottbell commented Mar 16, 2023

scottbell commented Mar 16, 2023

scottbell commented Mar 16, 2023 • edited

scottbell commented Mar 17, 2023

scottbell commented Feb 1, 2023 •

edited by akhenry

scottbell commented Feb 1, 2023 •

edited

codecov bot commented Feb 1, 2023 •

edited

scottbell commented Feb 6, 2023 •

edited

scottbell commented Feb 7, 2023 •

edited

scottbell commented Mar 16, 2023 •

edited