From: Mark Brown <broonie@kernel.org> To: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.de>, Shuah Khan <shuah@kernel.org>, alsa-devel@alsa-project.org, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v1 0/6] kselftest/alsa: pcm-test improvements Date: Thu, 1 Dec 2022 18:44:17 +0000 [thread overview] Message-ID: <Y4j2AS/UuLwqxARU@sirena.org.uk> (raw) In-Reply-To: <a55212fc-a676-2335-b861-94ba8d10f207@perex.cz> [-- Attachment #1: Type: text/plain, Size: 3025 bytes --] On Thu, Dec 01, 2022 at 06:42:22PM +0100, Jaroslav Kysela wrote: > The current code allows to override "test.time1 {} test.time2 {}" blocks in > the configuration files which is equivalent to "test { time1 {} time2 {} }". Right, I was leaving that in place but just renaming so that the intent of the test was clearer and expanding the standard coverage - trying to make it clearer what the test was trying to accomplish when someone comes along trying to do something later on. It did however cross my mind that we might be better off having the tests read from the config file be in addition to the standard tests rather than overriding them, I think that'd work out a lot clearer in the end. > This changeset will introduce configuration lookups like > "pcm.0.0.PLAYBACK.44k1.2.big {}" which creates another configuration > structure. The '.' (compound level delimiter) should not be used in the test > name. I see, we could use another delimiter there easily enough (though if we segregated the built in and loaded test configurations I'm not sure it'd matter so much). > My original idea for the next improvement was to parse the > "pcm.0.0.PLAYBACK.test" compound and gather the tests for the given pcm. If > this compound is missing, we can continue with the hard-coded defaults. While it is useful to be able to specify additional tests through configuration I don't think we should be relying on that for coverage, we should have a more substantial baseline of tests so that systems like KernelCI get reasonable coverage without having to get changes individually integrated for boards (and then wait for them to filter out into the trees being tested). It doesn't scale out so well over the number of systems that we might be running on, especially if we come up with new tests and have to loop back over existing boards, and isn't really idiomatic for kselftest. I'm also a bit worried about the way we currently override the built in tests, it creates additional potential for confusion when looking at results if the test might've been turned into something different by the configuration file. > About the skips - the test should probably keep to support also the exact > parameters. For example - if the hardware must support 6 channels, it should > not be a skip but an error. Everything may be broken, including the PCM > configuration refining. Yes, there's a tension there between hard coded tests and the explicitly specified per board ones. I think the solution here is to add two tests for things we read from the configuration file rather than just adding by default, one verifying that we managed to configure the settings we asked for and one for the actual test. > I just sent the patch with my changes for comments [1]. It's just the base > code which may be extended with your requirements. The skips may be > implemented using configuration field like 'skip_if_rate_error yes' or so. > Let me know, if I can stack your changes on top, or perhaps, you may be > willing to adapt them. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Mark Brown <broonie@kernel.org> To: Jaroslav Kysela <perex@perex.cz> Cc: Takashi Iwai <tiwai@suse.de>, alsa-devel@alsa-project.org, Shuah Khan <shuah@kernel.org>, linux-kselftest@vger.kernel.org Subject: Re: [PATCH v1 0/6] kselftest/alsa: pcm-test improvements Date: Thu, 1 Dec 2022 18:44:17 +0000 [thread overview] Message-ID: <Y4j2AS/UuLwqxARU@sirena.org.uk> (raw) In-Reply-To: <a55212fc-a676-2335-b861-94ba8d10f207@perex.cz> [-- Attachment #1: Type: text/plain, Size: 3025 bytes --] On Thu, Dec 01, 2022 at 06:42:22PM +0100, Jaroslav Kysela wrote: > The current code allows to override "test.time1 {} test.time2 {}" blocks in > the configuration files which is equivalent to "test { time1 {} time2 {} }". Right, I was leaving that in place but just renaming so that the intent of the test was clearer and expanding the standard coverage - trying to make it clearer what the test was trying to accomplish when someone comes along trying to do something later on. It did however cross my mind that we might be better off having the tests read from the config file be in addition to the standard tests rather than overriding them, I think that'd work out a lot clearer in the end. > This changeset will introduce configuration lookups like > "pcm.0.0.PLAYBACK.44k1.2.big {}" which creates another configuration > structure. The '.' (compound level delimiter) should not be used in the test > name. I see, we could use another delimiter there easily enough (though if we segregated the built in and loaded test configurations I'm not sure it'd matter so much). > My original idea for the next improvement was to parse the > "pcm.0.0.PLAYBACK.test" compound and gather the tests for the given pcm. If > this compound is missing, we can continue with the hard-coded defaults. While it is useful to be able to specify additional tests through configuration I don't think we should be relying on that for coverage, we should have a more substantial baseline of tests so that systems like KernelCI get reasonable coverage without having to get changes individually integrated for boards (and then wait for them to filter out into the trees being tested). It doesn't scale out so well over the number of systems that we might be running on, especially if we come up with new tests and have to loop back over existing boards, and isn't really idiomatic for kselftest. I'm also a bit worried about the way we currently override the built in tests, it creates additional potential for confusion when looking at results if the test might've been turned into something different by the configuration file. > About the skips - the test should probably keep to support also the exact > parameters. For example - if the hardware must support 6 channels, it should > not be a skip but an error. Everything may be broken, including the PCM > configuration refining. Yes, there's a tension there between hard coded tests and the explicitly specified per board ones. I think the solution here is to add two tests for things we read from the configuration file rather than just adding by default, one verifying that we managed to configure the settings we asked for and one for the actual test. > I just sent the patch with my changes for comments [1]. It's just the base > code which may be extended with your requirements. The skips may be > implemented using configuration field like 'skip_if_rate_error yes' or so. > Let me know, if I can stack your changes on top, or perhaps, you may be > willing to adapt them. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --]
next prev parent reply other threads:[~2022-12-01 18:44 UTC|newest] Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-11-30 0:06 [PATCH v1 0/6] kselftest/alsa: pcm-test improvements Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 1/6] kselftest/alsa: Refactor pcm-test to list the tests to run in a struct Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 2/6] kselftest/alsa: Report failures to set the requested sample rate as skips Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 3/6] kselftest/alsa: Report failures to set the requested channels " Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 4/6] kselftest/alsa: Don't any configuration in the sample config Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 5/6] kselftest/alsa: Provide more meaningful names for tests Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 0:06 ` [PATCH v1 6/6] kselftest/alsa: Add more coverage of sample rates and channel counts Mark Brown 2022-11-30 0:06 ` Mark Brown 2022-11-30 13:42 ` Mark Brown 2022-11-30 13:52 ` Takashi Iwai 2022-11-30 13:52 ` Takashi Iwai 2022-12-01 17:42 ` [PATCH v1 0/6] kselftest/alsa: pcm-test improvements Jaroslav Kysela 2022-12-01 18:44 ` Mark Brown [this message] 2022-12-01 18:44 ` Mark Brown 2022-12-01 19:06 ` Takashi Iwai 2022-12-01 19:06 ` Takashi Iwai 2022-12-01 20:29 ` Mark Brown 2022-12-01 20:29 ` Mark Brown 2022-12-02 7:52 ` Takashi Iwai 2022-12-02 7:52 ` Takashi Iwai 2022-12-02 7:54 ` Takashi Iwai 2022-12-02 7:54 ` Takashi Iwai 2022-12-02 8:56 ` Jaroslav Kysela 2022-12-02 8:56 ` Jaroslav Kysela 2022-12-02 13:22 ` Mark Brown 2022-12-02 13:22 ` Mark Brown
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Y4j2AS/UuLwqxARU@sirena.org.uk \ --to=broonie@kernel.org \ --cc=alsa-devel@alsa-project.org \ --cc=linux-kselftest@vger.kernel.org \ --cc=perex@perex.cz \ --cc=shuah@kernel.org \ --cc=tiwai@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.