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

add test certificate verification on new commit #421

Closed
wants to merge 2 commits into from

Conversation

antoinedang
Copy link
Contributor

Related Issue(s)

issue

Changes Made

  • Replaced github action hosted integration tests with a local integration test suite and verification of the testing on github actions.

Tests Done

  • this PR will test

Required Wiki Changes (if any)

  • workflow section

@antoinedang antoinedang force-pushed the antoine_integration_test_suite branch 6 times, most recently from 1308850 to 6f6bdc8 Compare December 7, 2023 19:48
@antoinedang antoinedang self-assigned this Dec 7, 2023
@antoinedang antoinedang added the enhancement New feature or request label Dec 7, 2023
@MrMondrian
Copy link
Contributor

did you add the encrypted bash scripts so that they can be edited by future members? I think its fine just to add the binary. but asking members to run random binaries is lowkey kinda sus tho. also the binary won't work on arm computers

@antoinedang
Copy link
Contributor Author

did you add the encrypted bash scripts so that they can be edited by future members? I think its fine just to add the binary. but asking members to run random binaries is lowkey kinda sus tho. also the binary won't work on arm computers

yeah i kept the encrypted bash script so it can be iterated. If i just keep the binary then the script is lost and if we update any of the tests or whatever or want to add selective testing it would have to be re-written from scratch, so I feel like it makes sense to keep them there in case. I can put them somewhere else if necessary

Good point for the ARM thing, didnt know about that. Seems like from this post neurobin/shc#33 its possible to compile to ARM, so I'll do that now just need someone with ARM to test it.

@antoinedang antoinedang force-pushed the antoine_integration_test_suite branch from 6332eac to d3e15dc Compare December 7, 2023 20:12
@MrMondrian
Copy link
Contributor

I think what you have here works but wouldn't it make more sense to try and use manually triggered actions to reduce minutes instead of locally running? I think maybe this is more of a last resort if we manage to blow through all our minutes even with manually running. This post explains how to trigger pr test from manually triggered workflows

@frompotenza what are your thoughts

@antoinedang
Copy link
Contributor Author

I think what you have here works but wouldn't it make more sense to try and use manually triggered actions to reduce minutes instead of locally running? I think maybe this is more of a last resort if we manage to blow through all our minutes even with manually running. This post explains how to trigger pr test from manually triggered workflows

@frompotenza what are your thoughts

Yeah no for sure, it would be good to have both as an option. Like either test it manually on github actions w/ a comment or run it locally. Do you want me to add that feature inside this PR or in a seperate one? I assumed it would be seperate but at once makes sense too

@MrMondrian
Copy link
Contributor

MrMondrian commented Dec 7, 2023

I think a separate PR is best. I would make a manually triggered workflow that takes in the PR number as input, fetches the data with the github api, gets the commit, checks it out, does the test, then uses this action to set the status https://github.com/myrotvorets/set-commit-status-action

@antoinedang
Copy link
Contributor Author

I think a separate PR is best. I would make a manually triggered workflow that takes in the PR number as input, fetches the data with the github api, gets the commit, checks it out, does the test, then uses this action to set the status https://github.com/myrotvorets/set-commit-status-action

sounds good, I'll add it to the board. Good to merge though?

@antoinedang
Copy link
Contributor Author

antoinedang commented Dec 7, 2023

I think a separate PR is best. I would make a manually triggered workflow that takes in the PR number as input, fetches the data with the github api, gets the commit, checks it out, does the test, then uses this action to set the status https://github.com/myrotvorets/set-commit-status-action

sounds good, I'll add it to the board. Good to merge though?

btw integration test failed cuz i merged noetic in so the test certificate is invalid now. I looked into checking the contents of the catkin_ws/src folder instead of using commit hash but generating a certificate with those contents took super long. Would definitely be better though for the future

@MrMondrian
Copy link
Contributor

MrMondrian commented Dec 7, 2023

sounds good, I'll add it to the board. Good to merge though?

Well if we add this we will have to enforce it and if it ends up being a challenge for new members it could hold us back. I would like to verify it works on arm first

@antoinedang antoinedang closed this Jan 5, 2024
@antoinedang
Copy link
Contributor Author

closing this because it won't apply to the new sim, we can re-visit it later on if necessary

@MrMondrian MrMondrian deleted the antoine_integration_test_suite branch February 7, 2024 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants