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

general: Add basic unit tests and CI #6

Merged
merged 2 commits into from
Feb 8, 2024

Conversation

christopher-davis-afs
Copy link
Contributor

Adds basic unit tests and CI. The tests are a placeholder for now, this PR just provides the infrastructure for unit tests.

CC: @shankari @AssemblyJohn @Dominik-K

@shankari
Copy link

This is also not running the newly added checks. And it is not even running the codecov action.
I only see the DCO check here...

.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
@shankari
Copy link

shankari commented Jan 23, 2024

@christopher-davis-afs the good news is that the tests are running. The bad news is that a file that you modified is failing the static code analysis. I don't see think that the particular line that is being flagged has changed, but it would be worthwhile to ensure that the check passes.

Also, would it be possible to include the incremental changes as separate commits? I had some feedback, and you have addresse it, but I don't see a single commit for that. So I have to look at all the files and wonder what else might have changed. Or basically do a full re-review.

@christopher-davis-afs
Copy link
Contributor Author

Screenshot 2024-01-23 at 9 00 03 AM

GitHub has a button for comparing changes when someone makes a push. See "Compare" here.

My action is still not running, and the code broken in the static code analysis check is something that I did not change at all.

@christopher-davis-afs
Copy link
Contributor Author

Actually, that file was changed by clang-format, but otherwise this issue exists on main.

@christopher-davis-afs christopher-davis-afs force-pushed the wip/cdavis/add-tests-and-ci branch 2 times, most recently from 0341a6d to c683f3e Compare January 23, 2024 14:43
@christopher-davis-afs
Copy link
Contributor Author

Fixed that issue

@shankari
Copy link

@christopher-davis-afs

GitHub has a button for comparing changes when someone makes a push. See "Compare" here.

That's interesting! I had not noticed that before. Is there a reason you prefer this over just adding a new commit to fix the comment? I generally prefer the new commit because then the commit history reflects the comments that were addressed.

Fixed that issue

Great! I am going to approve this now, but it looks like you resolved it primarily by reverting the clang changes to the file. Doesn't this mean that the clang check will fail when workflows are enabled? I don't see a recent passing run on the US-JOET repo (https://github.com/US-JOET/libslac/actions

@andistorm
Copy link
Contributor

When the pull request is ready for merging, please mark me as a reviewer.

@christopher-davis-afs
Copy link
Contributor Author

I believe this (and my other PRs) are ready, unless you have additional review comments?

@andistorm andistorm self-requested a review February 6, 2024 20:49
Copy link
Contributor

@andistorm andistorm left a comment

Choose a reason for hiding this comment

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

Looks good to me in general, with a few minor comments.

Could you provide a link to a successful run of this workflow in your fork?

.ci/build-kit/install_and_test.sh Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
.github/workflows/build_and_test.yaml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
tests/CMakeLists.txt Show resolved Hide resolved
tests/libslac_unit_test.cpp Outdated Show resolved Hide resolved
Signed-off-by: Christopher Davis <150722105+christopher-davis-afs@users.noreply.github.com>
Signed-off-by: Christopher Davis <150722105+christopher-davis-afs@users.noreply.github.com>
Copy link
Contributor

@andistorm andistorm left a comment

Choose a reason for hiding this comment

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

Cool 👍

@andistorm andistorm merged commit 38e1bba into EVerest:main Feb 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants