661406 - Reporting Structure of Setup Module / Tear Down Module Unclear

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Owen Coutts [:tallOwen]]

User-Agent:       Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 

As per our conversation lets add the test name to setup module and teardown module to disambiguate it.

Reproducible: Always

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Owen, when do you need this? I assume we should try to get this into Mozmill 1.5.4. 

Only one more comment. A test module can contain more than one test, especially what Clint has in mind for Mozmill 2 and the restart tests. Right now we also support that in Mozmill 1.5.x but it isn't used anymore on our side. We need a clean implementation of such a feature.

Comment 2

[cmtalbert]

This is a 1.5.4 only feature.  It's a slight tweak to the reporting and is totally unnecessary in 2.0 due to the refactored reporting.  So, yes, we're going to take it.  Owen, can you paste the JSON into this file from your test so I can mock up the change we are talking about?  THat way it will be easier for Henrik and everyone else following along to see what is changing.

Comment 3

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Not that it matters too much to our group upstairs, but if you keep SetupTest/TeardownTest, the same disambiguation (but with the test name instead of the filename) would be useful there.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Owen, can you please provide the requested information? Two weeks have passed meanwhile. Thanks.

Comment 5

[Owen Coutts [:tallOwen]]

Here is a link to a mozmill report: http://jsonviewer.stack.hu/#http://mozmill-release.brasstacks.mozilla.com/db/0017c80a72fdd58c8bb2cbfee31a2533.

The issue that we had talked about was that if the function name is "setup module" or tear down module" its ambiguous as to which function its refering to.

Comment 6

[cmtalbert]

Attachment: Patch

This is the patch that adds the detail.

I have no way to get the testname, but I can get you the filename, which should be good given the structure of the mozmill tests.  Note that due to the way mozmill 1.5.x works, this changes the output for everything - stdout as well as reporting.  I could gate it so that it only does this if the --report option is specified, but in that case, then everything will change.  There isn't much configurability allowed by the 1.5.x branch in this area.

Comment 7

[cmtalbert]

Attachment: Sample JSON report with these changes

Here's a sample of a JSON report with the changes in the patch.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So we now simply duplicating the filename from the given path and use it as prefix for setupModule and teardownModule? Why can't this be built-up in the dashboard? I don't really see any advantage in here. In both cases you would have to split strings to get the corresponding test module.

Comment 9

[Jeff Hammel]

Comment on attachment 547177 [details] [diff] [review]
Patch

wfm

Comment 10

[cmtalbert]

Comment on attachment 547178 [details]
Sample JSON report with these changes

(In reply to comment #8)
> So we now simply duplicating the filename from the given path and use it as
> prefix for setupModule and teardownModule? Why can't this be built-up in the
> dashboard? I don't really see any advantage in here. In both cases you would
> have to split strings to get the corresponding test module.
That's what I asked Owen originally.  He maintained that having some way to disambiguate the setupModule's from each other in the JSON would enable him to more easily query the ES database. That was the entire incentive.  

Henrik, I'll wait for a go-ahead from you and Owen before checking this in.

Setting proper flags for this on the JSON attachment...

Comment 11

[Owen Coutts [:tallOwen]]

This looks great. I think it makes it much more clear what the test failures are actually refering to in this case.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #11)
> This looks great. I think it makes it much more clear what the test failures
> are actually refering to in this case.

This is not what I see in the sample JSON report. It simply adds the module name and not the function name as prefix:

"name":"test_setupmodulereporting.js::setupModule",
"filename":"/home/ateam/projects/mozmill/mozmill/test/test_setupmodulereporting.js",

So how does it tell you to which test function it correlates to?

Comment 13

[cmtalbert]

(In reply to comment #12)
> (In reply to comment #11)
> > This looks great. I think it makes it much more clear what the test failures
> > are actually refering to in this case.
> 
> This is not what I see in the sample JSON report. It simply adds the module
> name and not the function name as prefix:
> 
> "name":"test_setupmodulereporting.js::setupModule",
> "filename":"/home/ateam/projects/mozmill/mozmill/test/
> test_setupmodulereporting.js",
> 
> So how does it tell you to which test function it correlates to?

Because your tests only have one test function in them, I think it's pretty clear.

Mozmill really doesn't know what the test function is before it runs it.  All it knows is the file it is working on.  It has a file of functions that it searches and it runs a setupModule before finding the next function that starts with test and running that.

We could try doing this in JS, but I doubt that we'll be able to get much more information out of it.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

So the initial spec is unclear for me. Owen, please exactly specify what you need and how it is used for.

Also if we would go this way, we should do the same for the test function itself, because it could also have the same name across different test modules.

Comment 15

[Owen Coutts [:tallOwen]]

I think this works. The main point of this bug is that if you say the name of this test is "setupModule" that is a bit of a misnomer because we have lots of setup modules.

Now that we have test_setupmodulereporting.js::setupModule we are saying the name of this function is the setup module in test_setupmodulereporting.js. Academically a unit test could have more then one function in it. Even if we don't ever do that it makes sense that the best we can describe a specific setup/teardown module is the file its in.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #12)
> "name":"test_setupmodulereporting.js::setupModule",
> "filename":"/home/ateam/projects/mozmill/mozmill/test/
> test_setupmodulereporting.js",

So why can't you retrieve the prefix from the filename which is available at the same level? I assume you don't want to pre-process the report?

Comment 17

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to comment #14)

> Also if we would go this way, we should do the same for the test function
> itself, because it could also have the same name across different test
> modules.

I agree with this.

(In reply to comment #15)

> Now that we have test_setupmodulereporting.js::setupModule we are saying the
> name of this function is the setup module in test_setupmodulereporting.js.
> Academically a unit test could have more then one function in it. Even if we
> don't ever do that it makes sense that the best we can describe a specific
> setup/teardown module is the file its in.

I agree with this too.

In terms of the entire system, a given function name is file::function. I'd be in favor of expressing it that way as a general rule, then letting us cut it down for display if necessary.

Re: the ongoing debate, this strikes me as a pretty standard denormalization task. Most systems have an easier time cutting a single value in two than having to combine values from different levels of hierarchy (or different docs entirely). It just doesn't come up much in RDBMS systems because queries/views do that combination and flatten the results for you.

When it's something you specifically want to index on, having what amounts to a calculated field with the key value is quite useful. That logic can't live in the display system because it's not the display system doing the indexing. At best, we could have done it with a document pre-processor, but it's such a predictable task that I'm in favor of just doing it by default.

If we really wanted to be comprehensive, the right answer is probably to save all three: naked filename, naked function name, combined field for indexing. But since it's easy to derive the first and second from the third, I think this approach is fine.

Comment 18

[cmtalbert]

So, it sounds like we will go with the patch I've got on this bug?

I'll plan to check it in on Monday if I hear no objections.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As I read Geo's comment correctly, he also agrees on to add the filename to the test function and not only do that for setupModule and teardownModule.

Comment 20

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Using the filename as part of the name for any function seems right for consistency's sake, yes. 

However, I'm willing to take the patch we have to solve the immediate problem. This bug did only mention setup/teardown.

Comment 21

[cmtalbert]

(In reply to comment #20)
> Using the filename as part of the name for any function seems right for
> consistency's sake, yes. 
> 
> However, I'm willing to take the patch we have to solve the immediate
> problem. This bug did only mention setup/teardown.

It's simple to add the file name to every field in the report.  I can do that before checking it in.

Henrik's right, I skipped over that when reading your comment.

Comment 22

[cmtalbert]

Attachment: Sample with the file name prepended to all functions in test

So, with Geo's comment, this is the JSON that will be produced.

The code is very simple and is as follows:
if (filename exists):
  testname = <filename>::<functionname>
else:
  testname = <functionname>

So as long as we can obtain the test file name (and I don't see any case where we couldn't) then we will report <filename>::<functionname>.  If an unusual case happens where we cannot get the filename for some reason then we will fall back to the old style of reporting where we simply output the functionname.  

If you split this field on the character '::' then you will be fine.  In the first case, you'll get a tuple of filename and functionname, in the second, you'll just get the functionname.

I'll land this now since it seems we're in agreement here.

Comment 23

[cmtalbert]

Landed on hotfix-1.5: https://github.com/mozautomation/mozmill/commit/56b488cbefbb82406c0c79db34b35a0259787749

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Shouldn't this also land on master? I don't see a reference yet.

Also, we should have a test on the python side for this bug. Right now the JS test doesn't test the added behavior at all. We could have used any of the existing JS tests for it.

Comment 25

[cmtalbert]

This isn't a master change.  As noted in comment 3, for master you can use a pluggable reporting callback to fashion the report however you'd like.  I believe (though I could be wrong) that we removed all the specialized reporting code from master in favor of pluggable modules going forward.

Yeah, the test is incomplete.  But, we have no python tests for 1.5.4.  The test is good enough to run and then hand check that the output is correct.  That's all we can do on 1.5.4.

I recommend going back to -->FIXED unless we do actually still have code in master for specialized reporting.  And if we do have that code, then we should file a bug to remove it into a pluggable report module.

Comment 26

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #25)
> This isn't a master change.  As noted in comment 3, for master you can use a
> pluggable reporting callback to fashion the report however you'd like.  I
> believe (though I could be wrong) that we removed all the specialized
> reporting code from master in favor of pluggable modules going forward.
[..]
> I recommend going back to -->FIXED unless we do actually still have code in
> master for specialized reporting.  And if we do have that code, then we
> should file a bug to remove it into a pluggable report module.

I would like to get feedback from Jeff first. If he agrees on it, yes please close this bug as fixed.

Comment 27

[Jeff Hammel]

Yes, I am very pro not taking this on master. I can discuss more why in length, but mostly what ctalbert said.