Skip to content

Add Testcase for EncodeDataValueWithoutValueProperty - #3579

Merged
marcschier merged 9 commits into
OPCFoundation:masterfrom
KircMax:KircMax/AddEncoderTestDataValue
Mar 6, 2026
Merged

marcschier merged 9 commits into
OPCFoundation:masterfrom
KircMax:KircMax/AddEncoderTestDataValue

Conversation

@KircMax

@KircMax KircMax commented Feb 24, 2026 •

Copy link
Copy Markdown
Contributor

Proposed changes

Describe the changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.

Related Issues

Types of changes

What types of changes does your code introduce?
Put an x in the boxes that apply. You can also fill these out after creating the PR.

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@codecov

codecov Bot commented Feb 24, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.11%. Comparing base (d25caff) to head (b5ad509).
⚠️ Report is 133 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #3579       +/-   ##
===========================================
+ Coverage   51.86%   66.11%   +14.24%     
===========================================
  Files         370      459       +89     
  Lines       78618    97715    +19097     
  Branches    13650    16375     +2725     
===========================================
+ Hits        40779    64601    +23822     
+ Misses      33705    28037     -5668     
- Partials     4134     5077      +943     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KircMax
KircMax marked this pull request as draft February 24, 2026 08:48
@KircMax

KircMax commented Feb 24, 2026 •

Copy link
Copy Markdown
Contributor Author

I need to investigate the failures in BrowseComplexTypesServerAsync
image

@KircMax

KircMax commented Feb 24, 2026

Copy link
Copy Markdown
Contributor Author
image Does not really seem to be related to my changes I guess (?)

@romanett

Copy link
Copy Markdown
Contributor

@KircMax in current master i think it the audit event fixed already

@KircMax

KircMax commented Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

@romanett I synched the fork today before I created that branch - it also shows "up to date with master":
image
what can I do?

@KircMax
KircMax marked this pull request as ready for review February 24, 2026 15:21

@romanett romanett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As this Library is like a reference implementation I dont like this change.
You can leave the test and maybe change the AdjustExpectedBoundaryValues to handle this statement in the spec:
https://reference.opcfoundation.org/Core/Part6/v105/docs/5.2.2.17

@romanett romanett changed the title add EncodeDataValueWithoutValueProperty Add Testcade for EncodeDataValueWithoutValueProperty Feb 27, 2026
@KircMax KircMax changed the title Add Testcade for EncodeDataValueWithoutValueProperty Add Testcase for EncodeDataValueWithoutValueProperty Mar 2, 2026
@KircMax KircMax changed the title Add Testcase for EncodeDataValueWithoutValueProperty Adjust Encoding for DataValue with PicoSeconds, Add Testcase for EncodeDataValueWithoutValueProperty Mar 2, 2026
@KircMax

KircMax commented Mar 2, 2026

Copy link
Copy Markdown
Contributor Author

@romanett should I also adjust the other encoders accordingly?
Json and Xml?

@romanett romanett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I had some issue wrapping my head around. With this Change still the Server would not encode Messages like stated in the spec. This is not right.

@KircMax KircMax changed the title Adjust Encoding for DataValue with PicoSeconds, Add Testcase for EncodeDataValueWithoutValueProperty Add Testcase for EncodeDataValueWithoutValueProperty Mar 3, 2026
@marcschier
marcschier merged commit d7d870d into OPCFoundation:master Mar 6, 2026
92 checks passed
jnsjll pushed a commit to jnsjll/UA-.NETStandard that referenced this pull request Mar 12, 2026
)

* add EncodeDataValueWithoutValueProperty

* adjust de and encoder for datavalue -> always handle sourcepico, etc.

* extend EncodeDataValueWithoutValueProperty test

* nullhandling is needed

* keep declaration as it was

* keep binarydecoder, adjust the test case

* add spec. comment

* keep binaryencoder
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.

Encoding of DataValue without Value has changed

3 participants