Correct writeback_state sequence test #450

noursaidi · 2022-09-09T10:46:58Z

Correct behaviour of writeback_state sequence test

noursaidi · 2022-09-12T12:34:00Z

@grafnu what needs to be done to fix the generated.md as below:

Looking at the other sequences, it should at least include the change config, e.g.:

1. Update config for writeback
 * Set "pointset.points.filter_alarm_pressure_status.set_value" = false
 * Set "pointset.points.filter_differential_pressure_setpoint.set_value" = 60
 * Set "pointset.points.filter_differential_pressure_sensor.set_value" = 15

And ideally it would include some of the metadata around the tests

invalidTarget.target_point = invalidTarget.target_value
failureTarget.target_point = failureTarget.target_value
appliedTarget.target_point = appliedTarget.target_value

grafnu · 2022-09-12T14:03:59Z

When you say "include some of the metadata" -- do you mean that the values are showing up in the config sent to the device, but not in the generated.md? Or is it that the values aren't even showing up in the config? Basically, what is "it" in that sentence?

…

On Mon, Sep 12, 2022 at 5:34 AM Noureddine ***@***.***> wrote: @grafnu <https://github.com/grafnu> what needs to be done to fix the generated.md as below: Looking at the other sequences, it should at least include the change config, e.g.: 1. Update config for writeback * Set "pointset.points.filter_alarm_pressure_status.set_value" = false * Set "pointset.points.filter_differential_pressure_setpoint.set_value" = 60 * Set "pointset.points.filter_differential_pressure_sensor.set_value" = 15 And ideally it would include some of the metadata around the tests invalidTarget.target_point = invalidTarget.target_value failureTarget.target_point = failureTarget.target_value appliedTarget.target_point = appliedTarget.target_value — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPD37QGJ6HEEK5SSNIRTV54PMHANCNFSM6AAAAAAQIR7MNM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

noursaidi · 2022-09-12T15:56:27Z

At the moment the values/config diffs don't show up in generated.md at all.

The specific values would not be documented (as they're device specific), rather what they represent should

So looking at one line from the example in my comment, Instead of showing something like
* Set "pointset.points.filter_alarm_pressure_status.set_value" = false.

The point filter_alarm_pressure_status.set_value and the value false both come from invalidTarget.target_point and invalidTarget.target_value in the Java code respectively.

Ideally, we would document in generated.md something generic e.g. * Set "pointset.points.INVALID_POINT.set_value" = INVALID_VALUE instead

grafnu · 2022-09-12T16:00:28Z

The fact that they aren't showing up at all is the first problem. The config diff should show all the differences. Let me pull the branch down and I'll take a look directly...

…

On Mon, Sep 12, 2022 at 8:56 AM Noureddine ***@***.***> wrote: At the moment the values/config diffs don't show up in generated.md at all. The specific values would not be documented (as they're device specific), rather what they represent should So looking at one line from the example in my comment, Instead of showing something like * Set "pointset.points.filter_alarm_pressure_status.set_value" = false. The point filter_alarm_pressure_status.set_value and the value false both come from invalidTarget.target_point and invalidTarget.target_value in the Java code respectively. Ideally, we would document in generated.md something generic e.g. * Set "pointset.points.INVALID_POINT.set_value" = INVALID_VALUE instead — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPDZUIQOJCRXXLKSLOQLV55HDNANCNFSM6AAAAAAQIR7MNM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

noursaidi · 2022-09-13T10:34:57Z

Thanks Trevor for you PR, seems to have fixed the bug so generated.md now accurately represents

But it's introduce an error with the single_scan and periodic_scan - Test failed: Unexpected non-semantic Date in semantic value calculation - relevant diffs and logs in this test run -> https://github.com/noursaidi/udmi/actions/runs/3044131852/jobs/4904263182#step:5:11915

grafnu

Oy, I think this needs a merge with master before review! (GitHub algorithms seem to be bad??!)

noursaidi · 2022-09-14T09:00:17Z

@grafnu made a mistake when I did a merge hence the issues you saw (I think GitHub's algos were fine)

There are still 249 modified files, but the bulk of these are the sequencer outputs in validator/sequences, I copied in my latest run. All other changed files are as expected and should be reviewed.

grafnu · 2022-09-14T12:18:10Z

bin/clone_model

@@ -5,7 +5,7 @@ mkdir -p $ROOT_DIR/sites
 cd $ROOT_DIR/sites 

 MODEL_DIR=udmi_site_model
-MODEL_VER=1.7
+MODEL_VER=808aa06fbe922b9f749d2a6b52ff03b6f9074b48


needs 1.8 tag

Will do, but you also need to create a 1.8 tag for that repo.

grafnu · 2022-09-14T12:20:46Z

validator/sequences/broken_config/config_pointset.attr

@@ -6,5 +6,5 @@
  "subType" : "config",
  "category" : "commands",


I think these should be added to .gitattributes to ignore the diffs so it's clear to the reviewer!

What files need to be included in this directory? Without any redaction, all the files will change with every run except for the sequence.md? Should we only include sequence.md and ignore the rest?

grafnu · 2022-09-14T14:13:07Z

Ok, tag 1.8 should be ready!

…

On Wed, Sep 14, 2022 at 6:03 AM Noureddine ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In bin/clone_model <#450 (comment)>: > @@ -5,7 +5,7 @@ mkdir -p $ROOT_DIR/sites cd $ROOT_DIR/sites MODEL_DIR=udmi_site_model -MODEL_VER=1.7 +MODEL_VER=808aa06fbe922b9f749d2a6b52ff03b6f9074b48 Will do, but you also need to create a 1.8 tag for that repo. — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPDZPUHEIR4JRTSIK5JDV6HEK5ANCNFSM6AAAAAAQIR7MNM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

grafnu · 2022-09-14T14:13:50Z

The files only change if they're copied over... I'm not sure what/why you're asking... ? They're just examples so it seems like the complete set would be useful. I think you might be misinterpreting the impact of the change I suggested...

…

On Wed, Sep 14, 2022 at 6:05 AM Noureddine ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In validator/sequences/broken_config/config_pointset.attr <#450 (comment)>: > @@ -6,5 +6,5 @@ "subType" : "config", "category" : "commands", What files need to be included in this directory? Without any redaction, all the files will change with every run except for the sequence.md? Should we only include sequence.md and ignore the rest? — Reply to this email directly, view it on GitHub <#450 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAIEPDYBG2NKEPEEJ5JRX6LV6HEP7ANCNFSM6AAAAAAQIR7MNM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

noursaidi · 2022-09-14T14:32:16Z

Great, CI test all clear now so should be mergeable. I've also made the requested to .gitattributes.

I was just stating all the files will be modified when they're copied over (because timestamps, etc), including for those tests which did not change. I was just wondering if all the files were needed or some redaction was needed, but as you said its good to have the complete set

grafnu · 2022-09-16T13:34:54Z

Nour, can you separate out all the "I don't care" log files into a separate PR and then either push that through or have this PR against that one? Even though they're not visible by default, it's near impossible to review this PR because of the sheer number of "must ignore" files, and there's ways around that!

validator/src/main/java/com/google/daq/mqtt/util/ConfigDiffEngine.java

grafnu · 2022-09-20T14:11:00Z

.github/workflows/testing.yml

@@ -111,6 +111,9 @@ jobs:
          more /tmp/sequencer.out
          diff -u /tmp/sequencer.out etc/sequencer.out
          diff -u /tmp/generated.md docs/specs/sequences/generated.md
+          ls -1 validator/sequences | xargs -I% diff -u \


be warned, what I discovered the other day is that the shell in GitHub Actions doesn't always process failure conditions correctly. Did you check that this actually fails the run when there's a problem? ( You could quickly check by tweaking the validator/sequences/%/sequence.md file)

Thanks, I've checked and it does fail when there is a difference or missing file.

…eback_test1

noursaidi and others added 4 commits September 9, 2022 10:50

fix writeback sequence test to work as expected

17f0676

Update generated.md

13dacfd

Adding test sequence sample out

1ed02c9

add ci check for validator/sequences

40fa743

noursaidi force-pushed the writeback_test1 branch from 8fe6571 to 40fa743 Compare September 9, 2022 13:17

change clone_model checkout target

37471cb

grafnu and others added 3 commits September 13, 2022 10:00

Update sequence-generator to work with property maps (#15)

6e76c83

new generated.md (but not comitted validator proofs)

be9b7fb

commit sequencer proofs (expect pass)

6649742

grafnu and others added 3 commits September 13, 2022 15:35

Fixing semantic dates for discovery (#16)

6ab4205

add sequencer output files

12a967c

sequencer.out

05b93a1

noursaidi marked this pull request as ready for review September 13, 2022 15:31

noursaidi requested a review from grafnu September 13, 2022 15:42

grafnu reviewed Sep 13, 2022

View reviewed changes

noursaidi force-pushed the writeback_test1 branch 2 times, most recently from c9a0697 to 05b93a1 Compare September 14, 2022 08:48

noursaidi added 2 commits September 14, 2022 09:54

Merge branch 'master' into writeback_test1

b32f1a1

restore gcp_device_logs

a990010

noursaidi requested a review from grafnu September 14, 2022 11:36

grafnu reviewed Sep 14, 2022

View reviewed changes

comments - clone model and gitattributes

04357df

noursaidi force-pushed the writeback_test1 branch from 3fc5cad to 04357df Compare September 14, 2022 13:27

noursaidi requested a review from grafnu September 16, 2022 10:27

noursaidi added 4 commits September 20, 2022 12:32

Merge branch 'master' into writeback_test1

65d9ed5

remove irrelevant log files

dd55bd5

remove extra valid_serial_no from sequencer.out

9de2072

sequencer.out

779f38a

noursaidi commented Sep 20, 2022

View reviewed changes

validator/src/main/java/com/google/daq/mqtt/util/ConfigDiffEngine.java Show resolved Hide resolved

Fix recusrion (#17)

f4e745b

grafnu approved these changes Sep 20, 2022

View reviewed changes

noursaidi added 4 commits September 20, 2022 15:48

test ci - expect failure

99a92e7

Merge branch 'writeback_test1' of github.com:noursaidi/udmi into writ…

d072d15

…eback_test1

flip validation directory and add generated.md

06d1437

generated.md extra line

5d1a6b7

noursaidi force-pushed the writeback_test1 branch from d3cee30 to 5d1a6b7 Compare September 20, 2022 19:45

noursaidi added 6 commits September 20, 2022 21:15

add endpoint_config (expect fail)

781955f

expect pass

d4718b4

fix

1ecc1c2

empty

9dca779

add logging to ci

448b5be

remove additional logging

6a81454

noursaidi merged commit 8d6fd1b into faucetsdn:master Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct writeback_state sequence test #450

Correct writeback_state sequence test #450

noursaidi commented Sep 9, 2022

noursaidi commented Sep 12, 2022

grafnu commented Sep 12, 2022 via email

noursaidi commented Sep 12, 2022

grafnu commented Sep 12, 2022 via email

noursaidi commented Sep 13, 2022

grafnu left a comment

noursaidi commented Sep 14, 2022

grafnu Sep 14, 2022

noursaidi Sep 14, 2022

grafnu Sep 14, 2022

noursaidi Sep 14, 2022

grafnu commented Sep 14, 2022 via email

grafnu commented Sep 14, 2022 via email

noursaidi commented Sep 14, 2022

grafnu commented Sep 16, 2022

grafnu Sep 20, 2022

noursaidi Sep 21, 2022

		@@ -6,5 +6,5 @@
		"subType" : "config",
		"category" : "commands",

Correct writeback_state sequence test #450

Correct writeback_state sequence test #450

Conversation

noursaidi commented Sep 9, 2022

noursaidi commented Sep 12, 2022

grafnu commented Sep 12, 2022 via email

noursaidi commented Sep 12, 2022

grafnu commented Sep 12, 2022 via email

noursaidi commented Sep 13, 2022

grafnu left a comment

Choose a reason for hiding this comment

noursaidi commented Sep 14, 2022

grafnu Sep 14, 2022

Choose a reason for hiding this comment

noursaidi Sep 14, 2022

Choose a reason for hiding this comment

grafnu Sep 14, 2022

Choose a reason for hiding this comment

noursaidi Sep 14, 2022

Choose a reason for hiding this comment

grafnu commented Sep 14, 2022 via email

grafnu commented Sep 14, 2022 via email

noursaidi commented Sep 14, 2022

grafnu commented Sep 16, 2022

grafnu Sep 20, 2022

Choose a reason for hiding this comment

noursaidi Sep 21, 2022

Choose a reason for hiding this comment