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

Fixes CI to run unit tests #42

Merged
merged 5 commits into from
Dec 5, 2023
Merged

Fixes CI to run unit tests #42

merged 5 commits into from
Dec 5, 2023

Conversation

gruyaume
Copy link
Contributor

@gruyaume gruyaume commented Dec 4, 2023

Description

This PR fixes the existing CI workflow to run unit tests recursively and addresses various problems identified by go test

@gab-arrobo
Copy link
Contributor

Can you also please update the dependabot extension to yml as part of this PR?

@gruyaume
Copy link
Contributor Author

gruyaume commented Dec 4, 2023

Can you also please update the dependabot extension to yml as part of this PR?

Done

@gab-arrobo
Copy link
Contributor

Can you also please update the dependabot extension to yml as part of this PR?

Done

Thanks!

@gab-arrobo
Copy link
Contributor

@gruyaume, why are the changes in nrfcache/nrf_cache_test.go needed? The unit-tests passes in my setup without having to make the changes in nrfcache/nrf_cache_test.go.

@gruyaume
Copy link
Contributor Author

gruyaume commented Dec 5, 2023

@gruyaume, why are the changes in nrfcache/nrf_cache_test.go needed? The unit-tests passes in my setup without having to make the changes in nrfcache/nrf_cache_test.go.

My bad, these were an artefact from testing (because they have sleeps in them and take a while to run). I re-added them.

@gab-arrobo
Copy link
Contributor

@gruyaume, why are the changes in nrfcache/nrf_cache_test.go needed? The unit-tests passes in my setup without having to make the changes in nrfcache/nrf_cache_test.go.

My bad, these were an artefact from testing (because they have sleeps in them and take a while to run). I re-added them.

Do you still need to make the import order change in nrfcache/nrf_cache_test.go? I am asking this because in my local setup, I can successfully run the unit tests without having to make any change in nrfcache/nrf_cache_test.go

@gruyaume
Copy link
Contributor Author

gruyaume commented Dec 5, 2023

@gruyaume, why are the changes in nrfcache/nrf_cache_test.go needed? The unit-tests passes in my setup without having to make the changes in nrfcache/nrf_cache_test.go.

My bad, these were an artefact from testing (because they have sleeps in them and take a while to run). I re-added them.

Do you still need to make the import order change in nrfcache/nrf_cache_test.go? I am asking this because in my local setup, I can successfully run the unit tests without having to make any change in nrfcache/nrf_cache_test.go

Indeed, this change is not "needed", it's a linting improvement. I can remove it from this PR if you prefer.

@gab-arrobo
Copy link
Contributor

@gruyaume, why are the changes in nrfcache/nrf_cache_test.go needed? The unit-tests passes in my setup without having to make the changes in nrfcache/nrf_cache_test.go.

My bad, these were an artefact from testing (because they have sleeps in them and take a while to run). I re-added them.

Do you still need to make the import order change in nrfcache/nrf_cache_test.go? I am asking this because in my local setup, I can successfully run the unit tests without having to make any change in nrfcache/nrf_cache_test.go

Indeed, this change is not "needed", it's a linting improvement. I can remove it from this PR if you prefer.

Are you planning to open a PR for linting improvements? If so, I would prefer to remove that change from this PR because those linting changes are not aligned to what this PR is about (Fix CI unit tests). Does it make sense?

Copy link
Contributor

@gab-arrobo gab-arrobo left a comment

Choose a reason for hiding this comment

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

+1 (tested changes locally)

@gab-arrobo gab-arrobo merged commit a5be567 into omec-project:master Dec 5, 2023
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.

2 participants