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

slicer/test: Support for custom packages #80

Closed

Conversation

woky
Copy link
Contributor

@woky woky commented Jun 21, 2023

Currently we only test with the embedded base-files files package. This
quite limits what we can test. But since commit a3315e3
("testutil/pkgdata: Create deb programatically") we have the ability to
define our own custom package content.

Add support for testing with custom packages. These can be defined in
each test case per archive name. The content can be defined directly in
test case definition with the help of testutil.MakeTestDeb().

The package content is wrapped in the testPackage structure. This
introduces one level of nesting in each test case package definition. In
future, we will add package metadata to the package definition, namely
version. So wrapping the package content in the structure now and later
just adding a new field to it the structure later will make the future
diff much smaller.

@woky
Copy link
Contributor Author

woky commented Jun 21, 2023

This PR depends on #86.

@woky woky added the Blocked Waiting for something external label Jun 21, 2023
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch from af647df to 77fa262 Compare June 22, 2023 10:38
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch 2 times, most recently from f674334 to d6b8ffd Compare August 29, 2023 15:34
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch 2 times, most recently from 3ab563f to 61d2770 Compare September 19, 2023 06:07
@cjdcordeiro cjdcordeiro added Priority Look at me first and removed Blocked Waiting for something external labels Sep 26, 2023
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch 6 times, most recently from 61d2839 to 2f2b810 Compare October 9, 2023 18:33
@woky woky added Blocked Waiting for something external and removed Priority Look at me first labels Oct 9, 2023
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch 2 times, most recently from 8fd4339 to 83e83eb Compare October 9, 2023 19:15
@rebornplusplus
Copy link
Member

I think this only depends on #86 now?

@cjdcordeiro
Copy link
Collaborator

I think this only depends on #86 now?

I think so too, and thus it is worth merging them given how small #86 is

Copy link
Member

@rebornplusplus rebornplusplus left a comment

Choose a reason for hiding this comment

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

Only reviewed 83e83eb. It looks good to me, except the comment about names I put in below.

internal/slicer/slicer_test.go Outdated Show resolved Hide resolved
@woky
Copy link
Contributor Author

woky commented Oct 12, 2023

I think this only depends on #86 now?

I think so too, and thus it is worth merging them given how small #86 is

Yes, this now depends on #86. But #86 is different feature, so I'm not really sure if I should merge it.

@woky woky added Priority Look at me first and removed Blocked Waiting for something external labels Oct 12, 2023
@cjdcordeiro
Copy link
Collaborator

I think this only depends on #86 now?

I think so too, and thus it is worth merging them given how small #86 is

Yes, this now depends on #86. But #86 is different feature, so I'm not really sure if I should merge it.

It can be rebased now though, since the 1st commit is already on main

Currently we only test with the embedded base-files files package. This
quite limits what we can test. But since commit a3315e3
("testutil/pkgdata: Create deb programatically") we have the ability to
define our own custom package content.

Add support for testing with custom packages. These can be defined in
each test case per archive name. The content can be defined directly in
test case definition with the help of testutil.MustMakeDeb().

The package content is wrapped in the testPackage structure. This
introduces one level of nesting in each test case package definition. In
future, we will add package metadata to the package definition, namely
version. So wrapping the package content in the structure now and later
just adding a new field to it the structure later will make the future
diff much smaller.
@woky woky force-pushed the pub/slicer-test-archives-custom-packages branch from 83e83eb to 0d5d78c Compare October 12, 2023 16:40
summary: "Custom archives with custom packages",
pkgs: map[string]map[string]testPackage{
"leptons": {
"electron": testPackage{
Copy link
Contributor

@niemeyer niemeyer Oct 20, 2023

Choose a reason for hiding this comment

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

Neither in the type definition nor here do we have a hint of what these strings actually mean. This also seems slightly misleading because the testPackage is actually a slice here, isn't it? No, it's a deb. So it's archive and packages.

We have @rebornplusplus above hinting as this issue too. I'm not sure his suggestion is the best idea either, but it would be nice to make things slightly cleaner. Any suggestions?

Copy link
Contributor Author

@woky woky Oct 22, 2023

Choose a reason for hiding this comment

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

@niemeyer I've applied @rebornplusplus's suggestion and in the end, I like the result. The definition now looks like:

	summary: "Custom archives with custom packages",
	archives: map[string]testArchiveData{
		"leptons": testArchiveData{
			packages: []testPackageData{{
				name: "electron",
				content: testutil.MustMakeDeb([]testutil.TarEntry{
					Dir(0755, "./"),
					Dir(0755, "./mass/"),

I've made archive packages a list instead of a map because one archive can in theory contain packages of the same name with different versions and/or architectures. It also reduces one level of indentation.

Note that there's also testArchive structure which implements Archive interface. That's why I suffixed the new structures with Data, though I admit it's kind of a fallback naming...

The changes are in a new fixup commit. Does it look better now?

@niemeyer niemeyer added the Reviewed Supposedly ready for tuning or merging label Oct 20, 2023
@cjdcordeiro cjdcordeiro removed the Reviewed Supposedly ready for tuning or merging label Oct 23, 2023
@cjdcordeiro cjdcordeiro removed the Priority Look at me first label Oct 26, 2023
@letFunny
Copy link
Collaborator

Per our offline discussion, I think our best way forward is to close this PR and discuss with @rebornplusplus to see if it is a priority right now, and how to amend the code here to make it compatible with the latest changes. We will take that discussion offline and we can always re-open the PRs or create new ones as we see fit.

@cjdcordeiro cjdcordeiro added the Decaying It's been a while, close or act on it label Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Decaying It's been a while, close or act on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants