mamedev / mame

MAME
https://www.mamedev.org/
Other
7.75k stars 1.95k forks source link

imgtool doesn't appear to be working in at least one test case. #7124

Open Firehawke opened 3 years ago

Firehawke commented 3 years ago

Here's a log of exact events. I've tested this three times (twice with PowerShell and once with Cmd), with identical results.

┌[fireh@NIGHTINGALE] [ docs-updates ↑3 ~2 -0 !]
└[C:\msys64\src\mame]> .\imgtool.exe create coco_jvc_rsdos testdisk.dsk

┌[fireh@NIGHTINGALE] [ docs-updates ↑3 ~2 -0 !]
└[C:\msys64\src\mame]> echo meow meow meow > meow.txt
┌[fireh@NIGHTINGALE] [ docs-updates ↑3 ~2 -0 !]
└[C:\msys64\src\mame]> type meow.txt
meow
meow
meow
┌[fireh@NIGHTINGALE] [ docs-updates ↑3 ~2 -0 !]
└[C:\msys64\src\mame]> .\imgtool.exe put coco_jvc_rsdos testdisk.dsk meow.txt

┌[fireh@NIGHTINGALE] [ docs-updates ↑3 ~2 -0 !]
└[C:\msys64\src\mame]> .\imgtool.exe dir coco_jvc_rsdos testdisk.dsk

Contents of testdisk.dsk:
------------------------------ -------- --------------- ------------------
------------------------------ -------- --------------- ------------------
       0 File(s)               0 bytes                          156672 bytes free

I've also loaded the disk into the CoCo 2 driver and been unable to find anything on the disk after the steps, so it's pretty clear nothing is being written. I haven't tried pulling anything off of an existing disk, but that may also be broken.

npwoods commented 3 years ago

Taking a closer look at this, it seems that Imgtool expected you to do the following:

.\imgtool.exe put coco_jvc_rsdos testdisk.dsk meow.txt meow.txt

This is because the final argument is interpreted as a target (in this case, a target filename) and as zero length source file list.

I'll be the first one to admit that it is quite lame that the only way I figured this out was by building and debugging the thing.

I'd be happy to do some old fashioned "bug bashing" and usability improvements, but I would only really want to partake in such an effort if I got regression testing support. Its hard to look at this code from ~1999-2000 and not want to modernize it, but I would only want to do so if I knew someone was going to be testing the thing.

stilett0 commented 3 years ago

@npwoods - to be honest, not intending to speak for the others but regarding regression testing support: it's my opinion that passingly few of the usual "testers" (@Firehawke , @Tafoid , and to a far lesser degree @stilett0 = myself) on the dev team have a deep familiarity with all the intricacies of imgtool, nor how to rigorously test it.

While we could always stand to better educate ourselves - Firehawke is further along than myself for certain, and I know Tafoid loves to be expert in all things MAME - I would maybe suggest instead as regression testers people who've recently added functionality to it, people like @fulivi , @shattered , @tlindner or @hadess - if they were interested, of course. Or other volunteers.

I would also long-term suggest consulting with @angelosa who allegedly has been looking into unit testing frameworks again.

@npwoods - Finally, I checked through the logs at here and MAMETesters, these might require another look if you're up to it. https://github.com/mamedev/mame/issues/693 (Closed) https://github.com/mamedev/mame/pull/2263#issuecomment-385975833 (see comment) https://github.com/mamedev/mame/pull/2541#issuecomment-320486271 (see comment) https://github.com/mamedev/mame/issues/5476 (Closed) https://github.com/mamedev/mame/issues/5694 (Open) https://github.com/mamedev/mame/issues/7124 (this one) https://mametesters.org/view.php?id=6576 (Confirmed) https://mametesters.org/view.php?id=6693 (Acknowledged)

shattered commented 3 years ago

I am using kyua + atf + lua scripting + pngcmp for regression tests of MAME drivers; haven't tried to set up imgtool test suite though.

hadess commented 3 years ago

Happy to add data to an existing test suite, but please make sure that I don't need to build the whole of MAME to build imgtool or run the test suite.

Firehawke commented 3 years ago

I'm coming back into this a bit late, but I'm 99.9% certain it'll get automated testing if there's enough of an overhaul to get it modernized (and just as importantly get the built-in help up to date on what it really wants)

I've already committed to writing docs on imgtool*. The literal only reason that's currently backburnered is that I don't want to write docs for a tool that may undergo substantial changes in the near future.

I'm also fine with doing automated testing scripts*. I don't know 100% of the machines that the tool supports, but I can work something out and I imagine Tafoid could eventually get that into his test suite once it's all in place.

Those commitments are, as always, subject to whether the tool is going to remain present. I'm not pushing for the removal of imgtool by any means, but it's certainly not something I can provide code for and any long-term commitment on my part is always going to be subject to whether there's interest from others.