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 Unit Tests for User Space Convertor #199

Conversation

estebanreyl
Copy link
Member

@estebanreyl estebanreyl commented May 26, 2023

What this PR does / why we need it:
This PR introduces tests for multiple functions in the user space convertor. This is meant to add to the long-term reliability and maintainability of the project. Part of #198

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?

Notes:
As part of this change, I had to introduce registry mocking for resolve/push/fetch functionality, as a part of this I added the ORAS library which has a nice library to use for the in-storage implementation. I also added a few Manifests and layers as part of this. Tried to keep it minimal as this is checked in. Let me know if there are any concerns. Also, I am not the best with Unit tests so any feedback is greatly appreciated.

ChangeLog

Bugfix

  • Added a small bugfix to builder to remove a small contention issue found while testing.

Tests

builder_engine.go

  • uploadManifestAndConfig -> Added test
  • getBuilderEngineBase -> Added test
  • isGzipLayer -> Added Test

builder_utils.go

  • fetch - Covered by fetchManifest and fetchConfig
  • fetchManifest -> Added test
  • fetchConfig -> Added test
  • fetchManifestAndConfig -> Seemed unnecessary
  • downloadLayer -> Added test
  • writeConfig -> Added test
  • getFileDesc -> Added test
  • uploadBlob -> Added test
  • uploadBytes -> Added test
  • buildArchiveFromFiles -> Left For later
  • addFileToArchive -> Left For later

builder.go

  • build -> Added test

overlaybd_builder.go

  • checkForConvertedLayer -> Added test
  • storeConvertedLayer -> Added test
    I am not currenly sure how best to add the remaining functions due to their use of the overlaybd binaries. In the near term these should be covered by unit tests but leaving as a future work item.

fastoci_builder.go
Similar problem to above. Leaving for later.

End to End tests

  • The existing CI seems like the best place to add end to end tests. Left for later.

Testing Tools

Local Remotes
This is an abstraction to interact with a local registry. The registry itself supports fetching, resolving, and pushes. See the local_registry.go file for more info. Along with this there is a mocks/registry folder which allows us to load stored images into the local registry for testing for now I've kept the added images small and restricted to hello-world but more can be added easily.

Filesystem interactivity
test_utils.go introduces RunTestWithTempDir which is a helper emulating the snapshotter tests that allows us to run a test with a temporary directory. This is useful for testing the filesystem interactions of several of the functions.

In Memory DB
In memory DB for testing DB related operations

@estebanreyl estebanreyl force-pushed the esrey/pr/userspace-convertor/unit-tests branch 2 times, most recently from bc0044f to 88eb1d4 Compare May 26, 2023 23:18
@estebanreyl estebanreyl force-pushed the esrey/pr/userspace-convertor/unit-tests branch from 88eb1d4 to ad6e185 Compare May 26, 2023 23:49
@liulanzheng
Copy link
Member

@WaberZhuang cc please

@BigVan
Copy link
Member

BigVan commented May 31, 2023

Amazing...

Copy link
Member

@liulanzheng liulanzheng left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@WaberZhuang WaberZhuang left a comment

Choose a reason for hiding this comment

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

LGTM

@liulanzheng liulanzheng merged commit 95ad9d6 into containerd:main Jun 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.

4 participants