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.

Sign up for GitHub

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

Jump to bottom

Fixed JsonCompare() function and switched to using it for json_test #217

Merged
merged 1 commit into from
Apr 23, 2024
Merged

Fixed JsonCompare() function and switched to using it for json_test #217

merged 1 commit into from
Apr 23, 2024

Conversation

craigcomstock
Copy link
Contributor

Ticket: ENT-11450
Changelog: none

@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

@craigcomstock craigcomstock marked this pull request as draft April 17, 2024 16:38
olehermanse
olehermanse reviewed Apr 18, 2024
View reviewed changes
tests/unit/test.h Outdated Show resolved Hide resolved
@craigcomstock
Copy link
Contributor Author

I added a negative test and debug logging to see how that might look in case someone wants to debug WHY JsonCompare() found two json's to not be equal

test_json_object_merge_deep: Starting test
   debug: JsonCompare() fails, primitive '/tmp/foo' not equal to '/tmp/foobad'
   debug: JsonArrayCompare() fails, children not equal
   debug: JsonObjectCompare(), for key 'value', child objects not equal
   debug: JsonObjectCompare(), for key 'cfbs:delete_files.filenames_bad', child objects not equal
   debug: JsonObjectCompare(), for key 'variables', child objects not equal
merged json  : {"variables":{"cfbs:delete_files.filenames_bad":{"value":["/tmp/foo","/tmp/bar","/tmp/bar","/tmp/baz"]}}}
expected json: {"variables":{"cfbs:delete_files.filenames_bad":{"value":["/tmp/foobad","/tmp/bar","/tmp/bar","/tmp/baz"]}}}
test_json_object_merge_deep: Test completed successfully.

The output is a bit "backwards" by noting the inner-most problem and then the elements outside of that in sequence to furthest out last.

@craigcomstock craigcomstock force-pushed the ENT-11450/master branch 2 times, most recently from ab85141 to 994be5a Compare April 18, 2024 20:56
@craigcomstock craigcomstock changed the title Refactored json test for re-use in another project @craigcomstock Fixed JsonCompare() function and switched to using it for json_test Apr 18, 2024
@craigcomstock craigcomstock changed the title @craigcomstock Fixed JsonCompare() function and switched to using it for json_test Fixed JsonCompare() function and switched to using it for json_test Apr 18, 2024
@craigcomstock craigcomstock marked this pull request as ready for review April 18, 2024 21:09
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

craigcomstock
craigcomstock commented Apr 18, 2024
View reviewed changes
libutils/json.c Outdated Show resolved Hide resolved
larsewi
larsewi reviewed Apr 19, 2024
View reviewed changes
libutils/json.c Outdated Show resolved Hide resolved
libutils/json.c Outdated Show resolved Hide resolved
libutils/json.c Outdated Show resolved Hide resolved
libutils/json.c Outdated Show resolved Hide resolved
larsewi
larsewi reviewed Apr 19, 2024
View reviewed changes
tests/unit/json_test.c Outdated Show resolved Hide resolved
tests/unit/json_test.c Outdated Show resolved Hide resolved
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

vpodzime
vpodzime reviewed Apr 22, 2024
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Very nice fixes and improvements! Thanks!

libutils/json.c Outdated Show resolved Hide resolved
libutils/json.c Outdated Show resolved Hide resolved
libutils/json.c Show resolved Hide resolved
tests/unit/json_test.c Show resolved Hide resolved
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

olehermanse
olehermanse previously approved these changes Apr 22, 2024
View reviewed changes
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

@olehermanse
Copy link
Contributor

@cf-bottom jenkins, please

@cf-bottom
Copy link

Sure, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/10628/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-10628/

@craigcomstock
Fixed JsonCompare() function and switched to using it for json_test
dc30808 
Also added DEBUG logging for details on why JsonCompare() found two json elements to not be equal.

Ticket: ENT-11450
Changelog: none
vpodzime
vpodzime previously approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

I'd do two things differently, but it's definitely good to go as is.

libutils/json.c Show resolved Hide resolved
tests/unit/json_test.c Show resolved Hide resolved
tests/unit/json_test.c Outdated Show resolved Hide resolved
olehermanse
olehermanse previously approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@olehermanse olehermanse left a comment

Choose a reason for hiding this comment

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

LGTM, but take a look at Jenkins test failures before merging.

@craigcomstock craigcomstock dismissed stale reviews from olehermanse and vpodzime via dc30808 April 23, 2024 14:03
@mender-test-bot
Copy link

There was an error running your pipeline, see logs for details.

@craigcomstock
Copy link
Contributor Author

@cf-bottom jenkins

vpodzime
vpodzime approved these changes Apr 23, 2024
View reviewed changes
Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

ACK

@cf-bottom
Copy link

Alright, I triggered a build:

Build Status

Jenkins: https://ci.cfengine.com/job/pr-pipeline/10629/

Packages: http://buildcache.cfengine.com/packages/testing-pr/jenkins-pr-pipeline-10629/

@craigcomstock
Copy link
Contributor Author

LGTM, but take a look at Jenkins test failures before merging.

One was a 52 exit code during cfbs test which is likely "known" and should already be addressed so maybe the fix for https://northerntech.atlassian.net/browse/ENT-11526 (apache graceful restart) didn't quite fix it.

The other is maybe new, I have logged a ticket: https://northerntech.atlassian.net/browse/ENT-11565

@olehermanse olehermanse merged commit 28df3c4 into NorthernTechHQ:master Apr 23, 2024
7 checks passed
@craigcomstock
Copy link
Contributor Author

PR to pull this change (and any others up to this merge) into core cfengine/core#5483

@craigcomstock craigcomstock deleted the ENT-11450/master branch April 23, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olehermanse olehermanse olehermanse approved these changes

@vpodzime vpodzime vpodzime approved these changes

@larsewi larsewi Awaiting requested review from larsewi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@craigcomstock @mender-test-bot @olehermanse @cf-bottom @vpodzime @larsewi