| Message ID | 20190416220449.25875-1-swarren@wwwdotorg.org |
|---|---|
| State | Accepted |
| Commit | 89a5317ae274991507931cb08f5142584561ec41 |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot] test/py: don't use mmc_rd config for other mmc tests | expand |
On 4/16/19 4:04 PM, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the > same configuration data that test_mmc_rd() does. Doing so causes the > following issues: > > * The new code uncondtionally expects certain keys to exist in the > configuration data. These keys do not exist in existing configuration > data since they were not previously required, and there was no > notification re: a requirement to add these new keys. This causes test > failures due to thrown exceptions when accessing the non-existent keys. > > * The new tests logically operate on different objects. test_mmc_rd() > operates on ranges of sectors on an MMC device (which may be the entire > set of sectors of a device, or a part of a device), whereas all the new > tests operate solely on entire devices. These are separate things, and > it's entirely likely that the user will wish to runs the two types of > tests on different sets of data; see the example configuration data that > this commit adds. Ideally, the new tests would have been added to a > separate Python file, since they aren' closely related to the existing > tests. > > FIXME: Marek, can you please replace the "???" in this patch with some > reasonable looking data? Thanks. Tom, Marek, any chance of applying this? Right now, every mainline branch is failing 5 tests on every push, which wastes my time having to go through all the logs to manually check that the failures aren't anything new/unknown. Thanks.
On 4/30/19 5:29 PM, Stephen Warren wrote: > On 4/16/19 4:04 PM, Stephen Warren wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the >> same configuration data that test_mmc_rd() does. Doing so causes the >> following issues: >> >> * The new code uncondtionally expects certain keys to exist in the >> configuration data. These keys do not exist in existing configuration >> data since they were not previously required, and there was no >> notification re: a requirement to add these new keys. This causes test >> failures due to thrown exceptions when accessing the non-existent keys. >> >> * The new tests logically operate on different objects. test_mmc_rd() >> operates on ranges of sectors on an MMC device (which may be the entire >> set of sectors of a device, or a part of a device), whereas all the new >> tests operate solely on entire devices. These are separate things, and >> it's entirely likely that the user will wish to runs the two types of >> tests on different sets of data; see the example configuration data that >> this commit adds. Ideally, the new tests would have been added to a >> separate Python file, since they aren' closely related to the existing >> tests. >> >> FIXME: Marek, can you please replace the "???" in this patch with some >> reasonable looking data? Thanks. > > Tom, Marek, any chance of applying this? Right now, every mainline > branch is failing 5 tests on every push, which wastes my time having to > go through all the logs to manually check that the failures aren't > anything new/unknown. Thanks. I'm still overloaded, and didn't have time to look into this. But I'm really not fond of the duplication here -- now we have two big tables describing very much the same thing, which SD/MMC to test .
On 4/30/19 10:27 AM, Marek Vasut wrote: > On 4/30/19 5:29 PM, Stephen Warren wrote: >> On 4/16/19 4:04 PM, Stephen Warren wrote: >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the >>> same configuration data that test_mmc_rd() does. Doing so causes the >>> following issues: >>> >>> * The new code uncondtionally expects certain keys to exist in the >>> configuration data. These keys do not exist in existing configuration >>> data since they were not previously required, and there was no >>> notification re: a requirement to add these new keys. This causes test >>> failures due to thrown exceptions when accessing the non-existent keys. >>> >>> * The new tests logically operate on different objects. test_mmc_rd() >>> operates on ranges of sectors on an MMC device (which may be the entire >>> set of sectors of a device, or a part of a device), whereas all the new >>> tests operate solely on entire devices. These are separate things, and >>> it's entirely likely that the user will wish to runs the two types of >>> tests on different sets of data; see the example configuration data that >>> this commit adds. Ideally, the new tests would have been added to a >>> separate Python file, since they aren' closely related to the existing >>> tests. >>> >>> FIXME: Marek, can you please replace the "???" in this patch with some >>> reasonable looking data? Thanks. >> >> Tom, Marek, any chance of applying this? Right now, every mainline >> branch is failing 5 tests on every push, which wastes my time having to >> go through all the logs to manually check that the failures aren't >> anything new/unknown. Thanks. > > I'm still overloaded, and didn't have time to look into this. But I'm > really not fond of the duplication here -- now we have two big tables > describing very much the same thing, which SD/MMC to test . There's no redundancy; the two different tests are semantically entirely different and don't share any configuration. See the patch description above for the full details.
On 5/1/19 4:51 PM, Stephen Warren wrote: > On 4/30/19 10:27 AM, Marek Vasut wrote: >> On 4/30/19 5:29 PM, Stephen Warren wrote: >>> On 4/16/19 4:04 PM, Stephen Warren wrote: >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the >>>> same configuration data that test_mmc_rd() does. Doing so causes the >>>> following issues: >>>> >>>> * The new code uncondtionally expects certain keys to exist in the >>>> configuration data. These keys do not exist in existing configuration >>>> data since they were not previously required, and there was no >>>> notification re: a requirement to add these new keys. This causes test >>>> failures due to thrown exceptions when accessing the non-existent keys. >>>> >>>> * The new tests logically operate on different objects. test_mmc_rd() >>>> operates on ranges of sectors on an MMC device (which may be the entire >>>> set of sectors of a device, or a part of a device), whereas all the new >>>> tests operate solely on entire devices. These are separate things, and >>>> it's entirely likely that the user will wish to runs the two types of >>>> tests on different sets of data; see the example configuration data >>>> that >>>> this commit adds. Ideally, the new tests would have been added to a >>>> separate Python file, since they aren' closely related to the existing >>>> tests. >>>> >>>> FIXME: Marek, can you please replace the "???" in this patch with some >>>> reasonable looking data? Thanks. >>> >>> Tom, Marek, any chance of applying this? Right now, every mainline >>> branch is failing 5 tests on every push, which wastes my time having to >>> go through all the logs to manually check that the failures aren't >>> anything new/unknown. Thanks. >> >> I'm still overloaded, and didn't have time to look into this. But I'm >> really not fond of the duplication here -- now we have two big tables >> describing very much the same thing, which SD/MMC to test . > > There's no redundancy; the two different tests are semantically entirely > different and don't share any configuration. See the patch description > above for the full details. Tom, could you please apply this patch to fix the test failures. Thanks.
On Tue, Apr 16, 2019 at 04:04:49PM -0600, Stephen Warren wrote: > From: Stephen Warren <swarren@nvidia.com> > > Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the > same configuration data that test_mmc_rd() does. Doing so causes the > following issues: > > * The new code uncondtionally expects certain keys to exist in the > configuration data. These keys do not exist in existing configuration > data since they were not previously required, and there was no > notification re: a requirement to add these new keys. This causes test > failures due to thrown exceptions when accessing the non-existent keys. > > * The new tests logically operate on different objects. test_mmc_rd() > operates on ranges of sectors on an MMC device (which may be the entire > set of sectors of a device, or a part of a device), whereas all the new > tests operate solely on entire devices. These are separate things, and > it's entirely likely that the user will wish to runs the two types of > tests on different sets of data; see the example configuration data that > this commit adds. Ideally, the new tests would have been added to a > separate Python file, since they aren' closely related to the existing > tests. > > FIXME: Marek, can you please replace the "???" in this patch with some > reasonable looking data? Thanks. > > Cc: Marek Vasut <marek.vasut@gmail.com> > Fixes: 4ffec8cdf512 ("test/py: mmc: Add 'mmc info' test") > Fixes: ce4b2cafa79c ("test/py: mmc: Add 'mmc rescan' test") > Fixes: 86dfd152c917 ("test/py: mmc: Add 'mmc dev' test") > Signed-off-by: Stephen Warren <swarren@nvidia.com> Applied to u-boot/master, thanks!
On 5/10/19 5:12 AM, Tom Rini wrote: > On Tue, Apr 16, 2019 at 04:04:49PM -0600, Stephen Warren wrote: > >> From: Stephen Warren <swarren@nvidia.com> >> >> Fix test_mmc_dev(), test_mmc_rescan(), test_mmc_info() not to use the >> same configuration data that test_mmc_rd() does. Doing so causes the >> following issues: >> >> * The new code uncondtionally expects certain keys to exist in the >> configuration data. These keys do not exist in existing configuration >> data since they were not previously required, and there was no >> notification re: a requirement to add these new keys. This causes test >> failures due to thrown exceptions when accessing the non-existent keys. >> >> * The new tests logically operate on different objects. test_mmc_rd() >> operates on ranges of sectors on an MMC device (which may be the entire >> set of sectors of a device, or a part of a device), whereas all the new >> tests operate solely on entire devices. These are separate things, and >> it's entirely likely that the user will wish to runs the two types of >> tests on different sets of data; see the example configuration data that >> this commit adds. Ideally, the new tests would have been added to a >> separate Python file, since they aren' closely related to the existing >> tests. >> >> FIXME: Marek, can you please replace the "???" in this patch with some >> reasonable looking data? Thanks. >> >> Cc: Marek Vasut <marek.vasut@gmail.com> >> Fixes: 4ffec8cdf512 ("test/py: mmc: Add 'mmc info' test") >> Fixes: ce4b2cafa79c ("test/py: mmc: Add 'mmc rescan' test") >> Fixes: 86dfd152c917 ("test/py: mmc: Add 'mmc dev' test") >> Signed-off-by: Stephen Warren <swarren@nvidia.com> > > Applied to u-boot/master, thanks! Thanks. Testing of u-boot/master is all back to green now.
diff --git a/test/py/tests/test_mmc_rd.py b/test/py/tests/test_mmc_rd.py index 2dc715bb5101..a25aa5f6f78e 100644 --- a/test/py/tests/test_mmc_rd.py +++ b/test/py/tests/test_mmc_rd.py @@ -13,6 +13,53 @@ import u_boot_utils This test relies on boardenv_* to containing configuration values to define which MMC devices should be tested. For example: +# Configuration data for test_mmc_dev, test_mmc_rescan, test_mmc_info; defines +# whole MMC devices that mmc dev/rescan/info commands may operate upon. +env__mmc_dev_configs = ( + { + 'fixture_id': 'emmc-boot0', + 'is_emmc': True, + 'devid': 0, + 'partid': 1, + 'info_device': ???, + 'info_speed': ???, + 'info_mode': ???, + 'info_buswidth': ???. + }, + { + 'fixture_id': 'emmc-boot1', + 'is_emmc': True, + 'devid': 0, + 'partid': 2, + 'info_device': ???, + 'info_speed': ???, + 'info_mode': ???, + 'info_buswidth': ???. + }, + { + 'fixture_id': 'emmc-data', + 'is_emmc': True, + 'devid': 0, + 'partid': 0, + 'info_device': ???, + 'info_speed': ???, + 'info_mode': ???, + 'info_buswidth': ???. + }, + { + 'fixture_id': 'sd', + 'is_emmc': False, + 'devid': 1, + 'partid': None, + 'info_device': ???, + 'info_speed': ???, + 'info_mode': ???, + 'info_buswidth': ???. + }, +} + +# Configuration data for test_mmc_rd; defines regions of the MMC (entire +# devices, or ranges of sectors) which can be read: env__mmc_rd_configs = ( { 'fixture_id': 'emmc-boot0', @@ -85,12 +132,12 @@ def mmc_dev(u_boot_console, is_emmc, devid, partid): assert good_response in response @pytest.mark.buildconfigspec('cmd_mmc') -def test_mmc_dev(u_boot_console, env__mmc_rd_config): +def test_mmc_dev(u_boot_console, env__mmc_dev_config): """Test the "mmc dev" command. Args: u_boot_console: A U-Boot console connection. - env__mmc_rd_config: The single MMC configuration on which + env__mmc_dev_config: The single MMC configuration on which to run the test. See the file-level comment above for details of the format. @@ -98,20 +145,20 @@ def test_mmc_dev(u_boot_console, env__mmc_rd_config): Nothing. """ - is_emmc = env__mmc_rd_config['is_emmc'] - devid = env__mmc_rd_config['devid'] - partid = env__mmc_rd_config.get('partid', 0) + is_emmc = env__mmc_dev_config['is_emmc'] + devid = env__mmc_dev_config['devid'] + partid = env__mmc_dev_config.get('partid', 0) # Select MMC device mmc_dev(u_boot_console, is_emmc, devid, partid) @pytest.mark.buildconfigspec('cmd_mmc') -def test_mmc_rescan(u_boot_console, env__mmc_rd_config): +def test_mmc_rescan(u_boot_console, env__mmc_dev_config): """Test the "mmc rescan" command. Args: u_boot_console: A U-Boot console connection. - env__mmc_rd_config: The single MMC configuration on which + env__mmc_dev_config: The single MMC configuration on which to run the test. See the file-level comment above for details of the format. @@ -119,9 +166,9 @@ def test_mmc_rescan(u_boot_console, env__mmc_rd_config): Nothing. """ - is_emmc = env__mmc_rd_config['is_emmc'] - devid = env__mmc_rd_config['devid'] - partid = env__mmc_rd_config.get('partid', 0) + is_emmc = env__mmc_dev_config['is_emmc'] + devid = env__mmc_dev_config['devid'] + partid = env__mmc_dev_config.get('partid', 0) # Select MMC device mmc_dev(u_boot_console, is_emmc, devid, partid) @@ -132,12 +179,12 @@ def test_mmc_rescan(u_boot_console, env__mmc_rd_config): assert 'no card present' not in response @pytest.mark.buildconfigspec('cmd_mmc') -def test_mmc_info(u_boot_console, env__mmc_rd_config): +def test_mmc_info(u_boot_console, env__mmc_dev_config): """Test the "mmc info" command. Args: u_boot_console: A U-Boot console connection. - env__mmc_rd_config: The single MMC configuration on which + env__mmc_dev_config: The single MMC configuration on which to run the test. See the file-level comment above for details of the format. @@ -145,13 +192,13 @@ def test_mmc_info(u_boot_console, env__mmc_rd_config): Nothing. """ - is_emmc = env__mmc_rd_config['is_emmc'] - devid = env__mmc_rd_config['devid'] - partid = env__mmc_rd_config.get('partid', 0) - info_device = env__mmc_rd_config['info_device'] - info_speed = env__mmc_rd_config['info_speed'] - info_mode = env__mmc_rd_config['info_mode'] - info_buswidth = env__mmc_rd_config['info_buswidth'] + is_emmc = env__mmc_dev_config['is_emmc'] + devid = env__mmc_dev_config['devid'] + partid = env__mmc_dev_config.get('partid', 0) + info_device = env__mmc_dev_config['info_device'] + info_speed = env__mmc_dev_config['info_speed'] + info_mode = env__mmc_dev_config['info_mode'] + info_buswidth = env__mmc_dev_config['info_buswidth'] # Select MMC device mmc_dev(u_boot_console, is_emmc, devid, partid)