Closed Bug 580057 Opened 14 years ago Closed 13 years ago

[report][manifests] mozmill logs should contain relative+canonical paths to tests instead of absolute paths

Categories

(Testing Graveyard :: Mozmill, defect)

Product:
Testing Graveyard
Component:
Mozmill
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: k0scist, Assigned: k0scist)

References

Details

(Whiteboard: [mozmill-2.0+])

Attachments

(1 file, 1 obsolete file)

take two
3.90 KB, patch
cmtalbert
: review+
Details | Diff | Splinter Review 
Currently, mozmill lists the entire path to file in the log output:

TEST-START | /home/jhammel/mozmill/src/mozmill-tests/firefox/testAwesomeBar/testLocationBarSearches.js | teardownModule

Instead, only the relative path to tests should be listed (e.g.):

TEST-START | firefox/testAwesomeBar/testLocationBarSearches.js | teardownModule
I plan the same thing for my work on bug 562822. Would any report module handle it on its own or could we generalize it.
As I have seen while working on bug 562822, we cannot do this inside Mozmill itself because Mozmill also supports other Gecko applications and we never know where those tests are located.

Instead I will do the conversation in my automation scripts for our QA usage. The same you should do for the build-bot integration.
"can't" is a rather strong word and depends on the constraint one wished to put upon the problem.  There are several things that could be done to improve the situation;  how canonical the results are depends on the solution and convention.

 Fix #1: Use only the relative path to the files from the `-t` specified from the command line.  For instance, if the user runs `mozmill -t /path/to/mozmill-tests/firefox`, it is relatively simple to display `testAwesomeBar/testLocationBarSearches.js` instead of the whole path that we display now, `/path/to/mozmill-tests/firefox/testAwesomeBar/testLocationBarSearches.js`.  While this doesn't provide a truly canonical path, it will at least provide the same results between computers with the same directory structures.  It is also noted, that except possibly for the `firefox` component (questionable), the path to mozmill-tests on different systems provides no interesting information to whoever is viewing the results.

 Fix #2: The lazy-man's fix for those that like to type alot.  Note that I don't advocate this idea, but it is easy refutation that this can't be done.  Each test can contain the full canonical path to the test:

"""
CANONICAL_NAME='firefox/testAwesomeBar/testLocationBarSearches.js'
"""

 This is rather silly, as it requires a large amount of manual work as well as high maintainence if tests are moved around, but of course it is possible.

 Fix #3: Record relative location of tests and determine the name from that. We already record RELATIVE_ROOT for module loading purposes:

"""
var RELATIVE_ROOT = '../../shared-modules';
"""

While this may/may not go away when Mozmill utilizes common.js, a similar approach may be used to determine the location of the test with respect to a (by convention) root. 

Of course, there are other ways this problem could be fixed as well, including manifests, path splitting, etc.  The point is, it can be made better.
This makes perfectly sense for our Firefox repository but customers of Mozmill who will test their own application will not be able to re-use such modified paths. Lemme give me an example.

FIREFOX
=======

CLI Options:
* You start a testrun with -t firefox
* You start a testrun with -t firefox/testAwesomeBar
* You start a testrun with -t firefox/testAwesomeBar/testLocationBarSearches.js

All those different options result in:
* Current Path: /Volumes/firefox/testAwesomeBar/testLocationBarSearches.js
* Canonical: firefox/testAwesomeBar/testLocationBarSearches.js

based on our information that 'firefox' is the top-level folder for Firefox tests we can strip everything in-front of the last existence of Firefox.

ADD-ONS
=======

CLI Options:
* You start a testrun with -t addons
* You start a testrun with -t addons/toolbar@google.com/

All those different options result in:
* Current Path: /Volumes/addons/toolbar@google.com/tests/testIsInstalled.js
* Canonical: addons/toolbar@google.com/tests/testIsInstalled.js

=> How can Mozmill decide where to strip off the leading path segments? We don't have anything to search for. Personally I'd be against adding an hardcoded value to Mozmill itself, to make it work with Firefox. At the same time we will break other apps. 

That's the reason why it is not part of my work on bug 562822.
Only the first fix (only printing the relative paths) will result in different results for these cases.  Since for Fix 2. and Fix 3. a true canonical path is determined (in Fix 2. it is stupidly hard coded, in Fix 3. the relativity is coded such that the canonical path may be determined), the examples given above will display the same path.

While I do not necessarily advocate fix one, though it does have the bonus that the testing structure is not changed, it at least gives the same results on different machines, or for that matter, on the same machine with different checkouts of e.g. mozmill-tests,  given the same `-t path/to/test` command line. I would say that this is at least an improvement.  If it is not good enough and this functionality is desired, we should go with Fix 2 or Fix 3.
We do have to keep in mind that the test structure in mozmill-tests is a purely Firefox QA created fiction.  Thunderbird has a slightly different structure and the calendar project has yet another different structure.

So, any idea that uses logic based on the firefox qa mozmill-test structure is going to be limiting us to that sandbox.  I'd rather the tool be widely versatile regardless of how people create their tests.  

Perhaps the collector could look at the set of tests that it is running and then strip off the common elements of the paths for reporting?

Or, we just keep reporting full paths and we allow the folks collecting the results to strip out things they don't care about.
(In reply to comment #6)
> Or, we just keep reporting full paths and we allow the folks collecting the
> results to strip out things they don't care about.

I would propose that way! We shouldn't be that smart with Mozmill. Everyone should decide on his own, how to map those paths. Otherwise we could risk that certain Xulrunner application test results could not be handled by their owners without modifying Mozmill.

As Clint told in the past, we also don't want to bind reporting too close to Couchdb. So lets continue that way and keep Mozmill clean. That said, I have removed the *smart* application branch detection on bug 562822 too.
I still find using absolute paths absolutely atrocious, for the record, but will go with group consensus.
Then it sounds like this is WONTFIX?
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Talked to Jeff some more and he has a good idea here that we should look at.

Basically, the reporting would be relative to the tail directory on the -t parameter.

i.e.

mozmill -t /blah/foo/baz/mytests

then the displayed file names would be:
* mytests/test1
* mytests/foodir/test2
* etc

If you use:
mozmill -t myfile.js
then you get this displayed:
* myfile.js

I think this does actually make a lot of sense and will keep the size of buildbot logs down, so we should look at that for a post 1.4.2 change.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(In reply to comment #10)
> Basically, the reporting would be relative to the tail directory on the -t
> parameter.
> 
> i.e.
> 
> mozmill -t /blah/foo/baz/mytests
> 
> then the displayed file names would be:
> * mytests/test1
> * mytests/foodir/test2
> * etc

Please don't do this otherwise we will loose all the information from the above directories. See this example:

> cd mozmill-tests/addons/
> mozmill -t toolbar@google.com/tests/

With that code we loose the information about the type of the run test.

Or does it mean that we only want to modify the paths below each single test result and the test path will still contain the absolute path?

report.test_path = "/Volumes/mozmill/default/addons/toolbar@google.com/tests/
report.test[x].path = tests/testDoThing.js
report.test[x].path = tests/testDoThing2.js
Firstly, I'm going to take a step back and say the following:  let's find the problem and fix that problem.  I'm not sure if we're doing that here.

The problem that I have is that because we print the absolute path to tests, the results cannot be compared even between checkouts of tests on the same machine to different paths, let alone different machines.  The other problem that I have is that the absolute paths contain information that is useless.  The current setup allows only comparison of runs given that the absolute path is constant.  This is hardly ideal.

(In reply to comment #11)
> (In reply to comment #10)
> > Basically, the reporting would be relative to the tail directory on the -t
> > parameter.
> > 
> > i.e.
> > 
> > mozmill -t /blah/foo/baz/mytests
> > 
> > then the displayed file names would be:
> > * mytests/test1
> > * mytests/foodir/test2
> > * etc
> 
> Please don't do this otherwise we will loose all the information from the above
> directories. See this example:
> 
> > cd mozmill-tests/addons/
> > mozmill -t toolbar@google.com/tests/
> 
> With that code we loose the information about the type of the run test.

I'm not sure what is losing this information.  Do you mean the information will not be reported correctly to a server?  Or to the person running the tests?  The person at the keyboard will presumedly know what tests they've run.  If it is the former issue, then how can one meaningfully compare the results between computers anyway?

> Or does it mean that we only want to modify the paths below each single test
> result and the test path will still contain the absolute path?
> 
> report.test_path = "/Volumes/mozmill/default/addons/toolbar@google.com/tests/
> report.test[x].path = tests/testDoThing.js
> report.test[x].path = tests/testDoThing2.js

So there are a few questions here and then several larger issues:

 - do we care about actual canonical paths?  we currently don't have them.  If we do care about them, I suggest getting them via manifests, which are already on the table for 2.0 anyway

 - does cutting off the path below that specified below -t break anything? what does it break?  does it break something that is now done as a workaround that instead should be done in a more robust way?  For my money, cutting off things below -t is quite fine.  It's *NOT* canonical, but it is better enough that I don't really care.

 - logging:  currently we culminate results two ways:
   1. via the python logging module and a combination of print statements to the screen which is controlled with --showall and --show-errors.  This is pretty ad hoc
   2. via JSON reports which get sent to a server
   Neither of these methods talks to the other.  this is...perty bad.  We should probably think of formalizing these interfaces, either making them modular things that plug into mozmill.  In general, we're dealing with hacks on top of hacks with respect to reporting.  We should probably figure out what we really want to do before doing too many more levels of hacks here.

AND we should figure out what the path to the tests should actually look like and make them look like that. BUT i'm pretty sure absolute paths isn't it.
(In reply to comment #12)
> I'm not sure what is losing this information.  Do you mean the information will
> not be reported correctly to a server?  Or to the person running the tests? 
> The person at the keyboard will presumedly know what tests they've run.  If it
> is the former issue, then how can one meaningfully compare the results between
> computers anyway?

It's about the identification on the server side. We know about the structure of our test repository. With that information we can strip down the paths. Means we (will) differentiate between:

* BFT tests: firefox/ and firefox/restartTests
* Update tests: firefox/softwareUpdate
* L10n tests: firefox/l10nTests
* Accessibility tests: firefox/accessTests
* Add-ons tests: addons/

With our automation scripts we make sure to always run the tests from an up-to-date copy of the repository. Given that we could strip down the paths to canonical ones in the scripts. It doesn't matter where the copy is located, because we can strip everything above firefox or addons.

Other projects would have their own rules and are eventually interested in having the full path information.

> > report.test_path = "/Volumes/mozmill/default/addons/toolbar@google.com/tests/
> > report.test[x].path = tests/testDoThing.js
> > report.test[x].path = tests/testDoThing2.js
> 
> So there are a few questions here and then several larger issues:
> 
>  - do we care about actual canonical paths?  we currently don't have them.  If
> we do care about them, I suggest getting them via manifests, which are already
> on the table for 2.0 anyway

Can you give me some more information about manifests? Does it mean that we want to use a .list file in each folder? If yes, how would that look like?

Our dashboard work hardly relies on canonical paths. But those we will create in our automation scripts.

>  - does cutting off the path below that specified below -t break anything? what
> does it break?  does it break something that is now done as a workaround that
> instead should be done in a more robust way?  For my money, cutting off things
> below -t is quite fine.  It's *NOT* canonical, but it is better enough that I
> don't really care.

Sorry. I don't understand that paragraph.

>  - logging:  currently we culminate results two ways:
>    1. via the python logging module and a combination of print statements to
> the screen which is controlled with --showall and --show-errors.  This is
> pretty ad hoc
>    2. via JSON reports which get sent to a server
>    Neither of these methods talks to the other.  this is...perty bad.  We
> should probably think of formalizing these interfaces, either making them
> modular things that plug into mozmill.  In general, we're dealing with hacks on
> top of hacks with respect to reporting.  We should probably figure out what we
> really want to do before doing too many more levels of hacks here.

I would be all up for that as long as we get the information we need on Brasstacks and we can make sure to not break testing of other applications.

> AND we should figure out what the path to the tests should actually look like
> and make them look like that. BUT i'm pretty sure absolute paths isn't it.

Agreed. But there should be at least a hook sub-classed Mozmill instances can use to determine what's the right behavior in their case.
(In reply to comment #13)
> (In reply to comment #12)
> > I'm not sure what is losing this information.  Do you mean the information will
> > not be reported correctly to a server?  Or to the person running the tests? 
> > The person at the keyboard will presumedly know what tests they've run.  If it
> > is the former issue, then how can one meaningfully compare the results between
> > computers anyway?
> 
> It's about the identification on the server side. We know about the structure
> of our test repository. With that information we can strip down the paths.
> Means we (will) differentiate between:
> 
> * BFT tests: firefox/ and firefox/restartTests
> * Update tests: firefox/softwareUpdate
> * L10n tests: firefox/l10nTests
> * Accessibility tests: firefox/accessTests
> * Add-ons tests: addons/
> 
> With our automation scripts we make sure to always run the tests from an
> up-to-date copy of the repository. Given that we could strip down the paths to
> canonical ones in the scripts. It doesn't matter where the copy is located,
> because we can strip everything above firefox or addons.

So in fact the reason that it is objected to printing test paths relative to the path passed to the command line is that there is a downstream processor that depends on the absolute path so that it can strip off the part of the path that is relative regardless of where it is run.

While this is a fine "this fixes things now" hack, it is this sort of thing that I would describe as not a good fix.  It depends on what I would call incorrect behaviour (having the absolute path to the test) and instead of fixing the root problem it is a workaround that uses a side-effect to its advantage.

I would prefer to fix this correctly.  If this means that (however its done, which there are at least four ways I can think of right now) we ensure that each test prints out the same test name regardless of what is passed to -t , then we should do that.

I don't see how printing out the absolute path can be considered correct behaviour and that the fact that it is used downstream is the only justification for this continuing.

> Other projects would have their own rules and are eventually interested in
> having the full path information.
> 
> > > report.test_path = "/Volumes/mozmill/default/addons/toolbar@google.com/tests/
> > > report.test[x].path = tests/testDoThing.js
> > > report.test[x].path = tests/testDoThing2.js
> > 
> > So there are a few questions here and then several larger issues:
> > 
> >  - do we care about actual canonical paths?  we currently don't have them.  If
> > we do care about them, I suggest getting them via manifests, which are already
> > on the table for 2.0 anyway
> 
> Can you give me some more information about manifests? Does it mean that we
> want to use a .list file in each folder? If yes, how would that look like?

jmaher's working on the format for mochitest.  Once there is a proof of concept, we'll work this into mozmill.

> Our dashboard work hardly relies on canonical paths. But those we will create
> in our automation scripts.
> 
> >  - does cutting off the path below that specified below -t break anything? what
> > does it break?  does it break something that is now done as a workaround that
> > instead should be done in a more robust way?  For my money, cutting off things
> > below -t is quite fine.  It's *NOT* canonical, but it is better enough that I
> > don't really care.
> 
> Sorry. I don't understand that paragraph.


Addressed above.

> >  - logging:  currently we culminate results two ways:
> >    1. via the python logging module and a combination of print statements to
> > the screen which is controlled with --showall and --show-errors.  This is
> > pretty ad hoc
> >    2. via JSON reports which get sent to a server
> >    Neither of these methods talks to the other.  this is...perty bad.  We
> > should probably think of formalizing these interfaces, either making them
> > modular things that plug into mozmill.  In general, we're dealing with hacks on
> > top of hacks with respect to reporting.  We should probably figure out what we
> > really want to do before doing too many more levels of hacks here.
> 
> I would be all up for that as long as we get the information we need on
> Brasstacks and we can make sure to not break testing of other applications.
> 
> > AND we should figure out what the path to the tests should actually look like
> > and make them look like that. BUT i'm pretty sure absolute paths isn't it.
> 
> Agreed. But there should be at least a hook sub-classed Mozmill instances can
> use to determine what's the right behavior in their case.

Agreed.  Someone should write a bug re: mozmill hooks.
> Agreed.  Someone should write a bug re: mozmill hooks.

And someone did!

bug 583936
(In reply to comment #14)
> > Can you give me some more information about manifests? Does it mean that we
> > want to use a .list file in each folder? If yes, how would that look like?
> 
> jmaher's working on the format for mochitest.  Once there is a proof of
> concept, we'll work this into mozmill.

Do we have a bug for that work? I would think so. We should add it to the dependency list.
Status: REOPENED → NEW
Depends on: 585106
Whiteboard: [mozmill-2.0?]
(In reply to comment #16)
> (In reply to comment #14)
> > > Can you give me some more information about manifests? Does it mean that we
> > > want to use a .list file in each folder? If yes, how would that look like?
> > 
> > jmaher's working on the format for mochitest.  Once there is a proof of
> > concept, we'll work this into mozmill.
> 
> Do we have a bug for that work? I would think so. We should add it to the
> dependency list.

I just added one, bug 585106 ; I'll flush out the other functionality we want out of manifests in their own bugs
Summary: mozmill logs should contain relative+canonical paths to tests instead of absolute paths → [report][manifests] mozmill logs should contain relative+canonical paths to tests instead of absolute paths
The default reporter functionality of mozmill should work using relative paths, just like the other test harnesses do.  If there is a user requirement for full paths, that can go into a pluggable event listener.
Whiteboard: [mozmill-2.0?] → [mozmill-2.0+]
Assignee: nobody → jhammel
Returning to this bug after a hiatus.  It seems like the first thing to do is to spec out what the name should be for tests.  Let's review the cases.

1. Invoking test.js in the CWD: mozmill -t test.js 
{'name': 'test.js'}
- this is pretty non-contraversial

2. Invoking test.js in a different directory: mozmill -t foo/bar/test.js
{'name': 'foo/bar/test.js'}
- this, OTOH, has lots of variants, including perhaps most importantly {'name': 'test.js'}. I could go either way, really. I'm suggesting {'name': 'foo/bar/test.js'} in anticipation of the complaint "what if I have multiple files called test.js in several locations?"

3. Invoking an entire test directory which contains foo.js and bar.js: mozmill -t test/directory
{'name': 'test/directory/bar.js'}, {'name': 'test/directory/foo.js'}
- this is to be consistent with 2. 

4. Invoking tests from the following manifest (mozmill -m manifest.ini)::
[test.js]
[test/directory/bar.js]
[test/directory/foo.js]
{'name': 'test.js'}, {'name': 'test/directory/bar.js'}, {'name': 'test/directory/foo.js'}
- this is already how manifests work.  Its fairly the same as 1-3 above
- Note that the names == section names; in other words, you will get the same names regardless of whether you use mozmill -m manifest.ini or mozmill -m subdir/manifest.ini; conceivably, you could change this, but....

5. Invoking tests with nested manifests.  Consider manifests joined with an [include: ] directive::

= main.ini =
[test.js]
[include:test/directory/manifest.ini]

= test/directory/manifest.ini =
[bar.js]
[foo.js]

- we could make this just take the section names from the manifest, requiring almost zero work:
{'name': 'test.js'}, {'name': 'bar.js'}, {'name': 'foo.js'}

- however, it might be more operator friendly to be based on the root manifest, in this case, main.ini:
{'name': 'test.js'}, {'name': 'test/directory/bar.js'}, {'name': 'test/directory/foo.js'}
This would require changes to manifestparser.py to keep track of this information.


Note that all of these options are dependent on the relative location from which you launch mozmill. This is by design.  There is no "one true name" of a test file, and absolute paths are no less arbitrary than a relative path.

Recall (important!) that multiple tests can be invoked from the command line.  This means that there is no global (relative) root directory.

Recall also (just as important!) that this is for convenience of reporting, not that any particular .js file has a UID.  This is already the case (just as important!). In other words, we can pretend that Volumes/mozmill/default/addons/toolbar@google.com/tests/ is the canonical location, but that will only be true for a few machines.  Now that reporting is pluggable, any sort of "canonizer" is possible for end use cases, but mozmill, the tool, shouldn't have canonization on this level.
Another thing to consider is how much this will change the mozmill architecture.  Mozmill.current_test is currently passed back from JS land. If we're going to use this to be what it says it is, then much more information should be passed to and from JS to keep the test name, which is only available on the python side.
(In reply to comment #20)

All of this sounds like the way to go.

> 5. Invoking tests with nested manifests.  Consider manifests joined with an
> [include: ] directive::
> 
> = main.ini =
> [test.js]
> [include:test/directory/manifest.ini]
> 
> = test/directory/manifest.ini =
> [bar.js]
> [foo.js]
> 
> - we could make this just take the section names from the manifest,
> requiring almost zero work:
> {'name': 'test.js'}, {'name': 'bar.js'}, {'name': 'foo.js'}
> 

I think this would be fine for now.
(In reply to comment #20)
> Returning to this bug after a hiatus.  It seems like the first thing to do
> is to spec out what the name should be for tests.  Let's review the cases.
> 
> 1. Invoking test.js in the CWD: mozmill -t test.js 
> {'name': 'test.js'}
> - this is pretty non-contraversial
yes

> 
> 2. Invoking test.js in a different directory: mozmill -t foo/bar/test.js
> {'name': 'foo/bar/test.js'}
> 
yes, do this.
> 3. Invoking an entire test directory which contains foo.js and bar.js:
> mozmill -t test/directory
> {'name': 'test/directory/bar.js'}, {'name': 'test/directory/foo.js'}
> - this is to be consistent with 2. 
>
yes.
 
> 4. Invoking tests from the following manifest (mozmill -m manifest.ini)::
> [test.js]
> [test/directory/bar.js]
> [test/directory/foo.js]
> {'name': 'test.js'}, {'name': 'test/directory/bar.js'}, {'name':
> 'test/directory/foo.js'}
> - this is already how manifests work.  Its fairly the same as 1-3 above
> - Note that the names == section names; in other words, you will get the
> same names regardless of whether you use mozmill -m manifest.ini or mozmill
> -m subdir/manifest.ini; conceivably, you could change this, but....
This is fine.

> 
> 5. Invoking tests with nested manifests.  Consider manifests joined with an
> [include: ] directive::
> 
> = main.ini =
> [test.js]
> [include:test/directory/manifest.ini]
> 
> = test/directory/manifest.ini =
> [bar.js]
> [foo.js]
> 
> - we could make this just take the section names from the manifest,
> requiring almost zero work:
> {'name': 'test.js'}, {'name': 'bar.js'}, {'name': 'foo.js'}
> 
> - however, it might be more operator friendly to be based on the root
> manifest, in this case, main.ini:
> {'name': 'test.js'}, {'name': 'test/directory/bar.js'}, {'name':
> 'test/directory/foo.js'}
> This would require changes to manifestparser.py to keep track of this
> information.
Depending on how much work it causes manifest parser.py I'd say we want to do the later, and reference everything relative to the root manifest. (which is honestly the same as scenario 4 - everything relative to the root manifest.

I really don't think it should cause any changes to the manifest parser because won't the manifest parser generate the json containing the path to the test file based on its path from the root manifest?  If it doesn't do that, then how would mozmill find that file in the first place?  So, I'm skeptical that there should need to be any change in the manifest parser.
(In reply to comment #23)
> (In reply to comment #20)
<snip/>
> > - however, it might be more operator friendly to be based on the root
> > manifest, in this case, main.ini:
> > {'name': 'test.js'}, {'name': 'test/directory/bar.js'}, {'name':
> > 'test/directory/foo.js'}
> > This would require changes to manifestparser.py to keep track of this
> > information.
> Depending on how much work it causes manifest parser.py I'd say we want to
> do the later, and reference everything relative to the root manifest. (which
> is honestly the same as scenario 4 - everything relative to the root
> manifest.
> 
> I really don't think it should cause any changes to the manifest parser
> because won't the manifest parser generate the json containing the path to
> the test file based on its path from the root manifest?  

It will calculate the path to (relative-pathed) tests based on the path to the manifest that it is in, not the "root" manifest: http://hg.mozilla.org/automation/ManifestDestiny/file/e9e1b1f64d3f/manifestparser.py#l428

In other words, if its all in one file, these are the same, but if you [include:] a file then they are different.

> If it doesn't do
> that, then how would mozmill find that file in the first place?  So, I'm
> skeptical that there should need to be any change in the manifest parser.

See above.  Its not a difficult change, but it is a change.

Consider this conundrum as well

= main.ini = 
[include:foo/manifest.ini]

= foo/manifest.ini =
[test.js]

mozmill -m main.ini -m foo/manifest.ini

Ignoring the fact that this is a silly thing to do, would you expect:
{'name': 'foo/test.js'}, {'name': 'test.js'} ?
Attached patch WIP (obsolete) — — Splinter Review 
So this works for most cases worth considering. We can continue on in this direction.  This currently doesn't touch reporting or manifestparser (or JS for that matter)
Attachment #537918 - Flags: feedback?(ctalbert)
Comment on attachment 537918 [details] [diff] [review]
WIP

Thinking about it more, I think we should take this (or some derivative, anyway) or resolve as won't fix. Reftest uses absolute paths.  While i think relative paths are much nicer for display and for comparison between systems, they're not the be-all end-all. I would like to take this patch.  But I can see not doing so too.  What we want, peoples? ;) </fickle>
Attachment #537918 - Flags: review?(ctalbert)
Attachment #537918 - Flags: feedback?(fayearthur+bugs)
Attachment #537918 - Flags: feedback?(ctalbert)
Comment on attachment 537918 [details] [diff] [review]
WIP

Review of attachment 537918 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks good.  I'm kind of ambivalent on the issue as well, but I think that going with relative paths would be nice since this way our test logs would have the same structure and be equivalent across OS's and regardless of the structure of the machine.  So, I'm happy to take it and switch to relative+canonical paths.

::: mozmill/mozmill/logger.py
@@ +177,2 @@
>      else:
> +      self.logger.log(self.custom_levels["TEST-PASS"], "%s | %s" % (filename, test['name']))

I have questions about this logic.  I think we just need name on both logging cases - send name when we pass and when we fail.  I'm not entirely sure what this filename variable is for and why you need it here.

We should write the data out the same way regardless of pass/fail.

r-
Attachment #537918 - Flags: review?(ctalbert) → review-
Attachment #537918 - Flags: feedback?(fayearthur+bugs)
Attached patch take two — — Splinter Review 
fixed mistake and unbitrotted
Attachment #537918 - Attachment is obsolete: true
Attachment #539914 - Flags: review?(ctalbert)
Comment on attachment 539914 [details] [diff] [review]
take two

thanks
Attachment #539914 - Flags: review?(ctalbert) → review+
pushed to master: https://github.com/mozautomation/mozmill/commit/f09fbb055c363170a9248214c9b8d5d3c38adb3f
Status: NEW → RESOLVED
Closed: 14 years ago → 13 years ago
Resolution: --- → FIXED
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: