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

[INFRA-340] Replace fail_unless with ck_assert_msg #900

Merged
merged 3 commits into from
Jan 13, 2021

Conversation

reimerix
Copy link
Contributor

@reimerix reimerix commented Dec 21, 2020

This PR replaces most of the fail_unless usages with ck_assert_msg, the former has been deprecated for a use with a message in the check library.

Most of the format warnings in the generated C tests are gone. However, a couple of messages like

warning: format '%d' expects argument of type 'int', but argument 5 has type 'u64 {aka long unsigned int}

remain.

I noticed that some of the auto tests were not re-generated and deleted the respective tests. Hence the large number of changed lines.

@silverjam
Copy link
Contributor

A little concerned that 40k lines where deleted, which looks like a fair number of tests were deleted-- I think @jbangelo ran in to a similar issue with the rust tests?

@jbangelo
Copy link
Contributor

I did run into similar behavior where rust test cases weren't getting re-generated. I think I ended up deleting the rust test files that were being generated and didn't root cause the issue. Looks like that is what has been done here as well?

@reimerix
Copy link
Contributor Author

Looks like that is what has been done here as well?

@jbangelo yes, exactly.

this at least brings rust in lock-step with c
Copy link
Contributor

@silverjam silverjam left a comment

Choose a reason for hiding this comment

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

@reimerix @jbangelo I looked through the Python code that generates the test files and what's generated (in auto generated tests) seems in line with what we have in git for YAML "test specification" that generate these files. That is, each test specification that references an SBP module (e.g. sbp.acquisition or sbp.tracking) causes a file to be generated. In the case of the sbp.acquisition there are 3 test spec YAMLs that reference this, so there are three files that are generated for Rust and C. Rust in fact had some more files that were no longer being generated (I fixed this by deleting everything in git, then regenerating, and re-adding).

Copy link
Contributor

@jbangelo jbangelo left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Thanks @silverjam for root causing the test generation issue. I guess we just need to be more careful about removing test code when test cases in the YAML are removed?

@silverjam
Copy link
Contributor

Changes look good to me. Thanks @silverjam for root causing the test generation issue. I guess we just need to be more careful about removing test code when test cases in the YAML are removed?

I think a future improvement could be to make sure that all auto generated files get touched somehow. By (for example) creating a manifest to track generated files and diff'ing that against what's on disk.

@silverjam
Copy link
Contributor

Changes look good to me. Thanks @silverjam for root causing the test generation issue. I guess we just need to be more careful about removing test code when test cases in the YAML are removed?

I think a future improvement could be to make sure that all auto generated files get touched somehow. By (for example) creating a manifest to track generated files and diff'ing that against what's on disk.

Created https://swift-nav.atlassian.net/browse/INFRA-347 to track

@silverjam silverjam merged commit e3acade into master Jan 13, 2021
@silverjam silverjam deleted the reimerix/infra-340/format-strings branch January 13, 2021 01:03
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.

3 participants