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

beluga_system_test fails to pass #154

Closed
glpuga opened this issue Mar 29, 2023 · 10 comments
Closed

beluga_system_test fails to pass #154

glpuga opened this issue Mar 29, 2023 · 10 comments
Labels
bug Something isn't working infra Related to infrastructure and CI

Comments

@glpuga
Copy link
Collaborator

glpuga commented Mar 29, 2023

Required info

Steps to reproduce issue

colcon build && colcon test

Expected behavior

In main I'd expect the test to pass.

Actual behavior

The test is unsuccessful.

Screenshot from 2023-03-28 23-08-34

@glpuga glpuga added the bug Something isn't working label Mar 29, 2023
@nahueespinosa
Copy link
Member

#142 introduced the usage of git-lfs in this repository. The missing bagfile is probably related to that.

On latest main, I do get another error:

build/beluga_system_tests/test_results/beluga_system_tests/clang_format.xunit.xml: 1 test, 1 error, 0 failures, 0 skipped
- beluga_system_tests clang_format.xunit.missing_result
  <<< error message
    The test did not generate a result file:
    
    Could not find config file '.clang-format'
  >>>
build/beluga_system_tests/test_results/beluga_system_tests/clang_tidy.xunit.xml: 1 test, 1 error, 0 failures, 0 skipped
- beluga_system_tests clang_tidy.xunit.missing_result
  <<< error message
    The test did not generate a result file:
    
    Could not find config file '.clang-tidy'
  >>>

Summary: 776 tests, 2 errors, 2 failures, 154 skipped

@nahueespinosa
Copy link
Member

nahueespinosa commented Mar 29, 2023

Should we consider not using git-lfs to avoid this kind of issue? @ivanpauno

@glpuga
Copy link
Collaborator Author

glpuga commented Mar 29, 2023

Should we consider not using git-lfs to avoid this kind of issue? @ivanpauno

@nahueespinosa alternatively, we can check if that is setup from the run.sh script.

nahueespinosa added a commit that referenced this issue Mar 29, 2023
Related to #154.

The configuration for `clang_format` and `clang_tidy` was moved to the
root of the repository in #133.
This patch removes the call to both `ament_clang_format` and
`ament_clang_tidy` from the system test package.

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
@nahueespinosa nahueespinosa added the infra Related to infrastructure and CI label Mar 29, 2023
@nahueespinosa
Copy link
Member

Sounds good to me!

@ivanpauno
Copy link
Collaborator

#142 introduced the usage of git-lfs in this repository. The missing bagfile is probably related to that.

I think it's good to use it, but we don't have enough binaries for it to be really important.
We're probably going to need it at some point if we start uploading more datasets for testing.

@nahueespinosa alternatively, we can check if that is setup from the run.sh script.

SGTM!


I think the main problem is that we weren't using it before, and now we're.
In that case you need to install git-lfs, install the lfs git hooks, and update the local repo to use the hook.
The dockerfile was updated to include git-lfs, so it shouldn't be a problem to commit from inside the container (after rebuilding it).
The contributing guidelines were also updated, so it shouldn't be a problem for someone doing a fresh start.

I think it's also okay to remove git-lfs though.

@nahueespinosa
Copy link
Member

Hard limit for files is 100MB.

https://docs.github.com/en/repositories/working-with-files/managing-large-files/about-large-files-on-github.

Our bagfile's size is 7.29MB, I'm not sure if we will want to store full datasets in this repo.
For testing purposes, we can always trim the dataset/rosbag to what is needed for a specific test case.

I'd remove git-lfs for now.

@ivanpauno
Copy link
Collaborator

I'd remove git-lfs for now.

SGTM!

@glpuga
Copy link
Collaborator Author

glpuga commented Mar 29, 2023

SGTMT.

@olmerg
Copy link
Collaborator

olmerg commented Apr 3, 2023

Maybe put in assets the data required to maintain small the repo

@glpuga
Copy link
Collaborator Author

glpuga commented Apr 17, 2023

I think we can close this one. The question regarding using LFS can get it's own focused ticket.

nahueespinosa added a commit that referenced this issue Apr 17, 2023
Related to #154.

As the title says, 15MB doesn't justify using Git LFS yet.

Signed-off-by: Nahuel Espinosa <nespinosa@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working infra Related to infrastructure and CI
Projects
None yet
Development

No branches or pull requests

4 participants