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

Address review comments from PR 24986 #24992

Closed
bzbarsky-apple opened this issue Feb 11, 2023 · 6 comments · Fixed by #25030
Closed

Address review comments from PR 24986 #24992

bzbarsky-apple opened this issue Feb 11, 2023 · 6 comments · Fixed by #25030

Comments

@bzbarsky-apple
Copy link
Contributor

Review comments there were not addressed before it was merged. See #24986 (comment)

Specific actions:

  1. Undo the changes to the certification tests that were not accompanied by test plan changes.
  2. Add the test that was asked for in the review comments.

@jrhees-cae @yufengwangca

@bzbarsky-apple
Copy link
Contributor Author

Of course ideally the test plans would in fact be modified so they catch this issue and then the test would be one of the certification tests.... @cjandhyala

jrhees-cae added a commit to jrhees-cae/connectedhomeip that referenced this issue Feb 13, 2023
- verify correct CredentialIndex on LockOperation events
- verify NULL UserIndex and Credentials on LockOperation errors when using invalid PINCodes
Fixes project-chip#24992
@cjandhyala cjandhyala reopened this Feb 14, 2023
@cjandhyala
Copy link
Contributor

@jrhees-cae Could you update the test plans, if the expected outcome in a TC is modified , we must execute the TC in a TE or SVE.

@jrhees-cae
Copy link
Contributor

@cjandhyala I looked at the test plan document related to this test case (DRLK-10) and there aren't any needed changes. The test plan does not specifically call out what UserIndex or CredentialIndex to use for the test (the only thing I changed was the credentialIndex used during the steps).

@cjandhyala
Copy link
Contributor

@jrhees-cae Ok, I see verification steps are updated to verify certain expected values, since the expected outcome is changed in some of the steps in the script, will need to run this script in a TE/SVE.

I see the script is updated to verify the below values.

  1. CredentialIndex: 1 is changed to CredentialIndex: 2
  2. verify "UserIndex and Credentials are set to null"

Also I see that expected outcome in one of the step is 'DataIndex is set to 4' but in the actual verification that value is not verified. Is that fine ?

@jrhees-cae
Copy link
Contributor

jrhees-cae commented Feb 14, 2023 via email

@bzbarsky-apple
Copy link
Contributor Author

Fixed by #25030

lecndav pushed a commit to lecndav/connectedhomeip that referenced this issue Mar 22, 2023
…nd userIndex (project-chip#24992) (project-chip#25030)

* Add tests in DL_LockUnlock yaml: (project-chip#24992)
- verify correct CredentialIndex on LockOperation events
- verify NULL UserIndex and Credentials on LockOperation errors when using invalid PINCodes
Fixes project-chip#24992

* Restyled by prettier-yaml

* Exclude DL_LockUnlock yaml test from Darwin (waiting on readEvent support)

---------

Co-authored-by: Restyled.io <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants