Ticket #1061 (closed cage: fixed)

Opened 12 years ago

Last modified 12 years ago

Lists of test files duplicated.

Reported by: coke Owned by: jkeenan
Priority: minor Milestone:
Component: testing Version: trunk
Severity: medium Keywords:
Cc: allison Language:
Patch status: applied Platform:

Description

We have lists of test files both in the root Makefile: 

+EXTRA_TEST_FILES := \
+    t/compilers/pct/*.t \
+    t/compilers/pge/*.t \
+    t/compilers/pge/p5regex/*.t \
+    t/compilers/pge/perl6regex/*.t \
+    t/compilers/tge/*.t \
+    t/library/*.t \
+    t/tools/*.t

and in lib/Parrot/Harness/DefaultTests.pm: 

@library_tests = qw(
    t/compilers/pct/*.t
    t/compilers/pge/*.t
    t/compilers/pge/p5regex/*.t
    t/compilers/pge/perl6regex/*.t
    t/compilers/tge/*.t
    t/library/*.t
    t/tools/*.t
);

Some are similarly named, some are differently named.

This information shouldn't be duplicated in 2 locations. I recommend not using the perl module to specify this via t/harness, but instead to declare the lists solely in the Makefile and then combine them as necessary if chunks of tests files are shared across test invocations.

If there's a special testing target we need, we can create a target for it rather than adding another command line option to t/harness.

Attachments

library_files.trunk.diff Download (14.6 KB) - added by jkeenan 12 years ago.
svn diff between trunk (at branch point) and library_files branch

Change History

  Changed 12 years ago by jkeenan

  • status changed from new to assigned
  • owner set to jkeenan

I will look into this. I don't recall seeing @library_tests in DefaultTests.pm when I refactored it a couple of years back, so maybe it crept in since then.

kid51

follow-up: ↓ 3   Changed 12 years ago by mikehh

The @library_tests were added at r40512 by allison and this moved these tests out of the core_tests and hence out of make fulltest. The EXTRA_TEST_FILES in root.in was my attempt to add these tests to make fulltest and make cover. There is probably a better way to do this, but I couldn't think of one at the moment.

in reply to: ↑ 2 ; follow-up: ↓ 4   Changed 12 years ago by jkeenan

  • cc allison added

Replying to mikehh:

The @library_tests were added at r40512 by allison and this moved these tests out of the core_tests and hence out of make fulltest.

Mike,

I discovered this just seconds before seeing your post: 

$ svn blame lib/Parrot/Harness/DefaultTests.pm | grep library_tests
 40512    allison     @library_tests
 40512    allison     @library_tests,
 40512    allison     @library_tests
 40512    allison @library_tests = qw(
 40512    allison            push @default_tests, @library_tests;

r40512 | allison | 2009-08-12 23:24:27 -0400 (Wed, 12 Aug 2009) | 4 lines

[build] Alter 'coretest' target so it only builds and tests the 
core vm.  Add a 'corevm' target as a subset of the 'all' target 
that only builds the core vm. The default build target is still 'all'.

But, are you sure that Allison's revision pulls these tests out of make fulltest? After all, if they're added to the list of default tests, they'd still be run as part of make test and hence as part of make fulltest -- correct?

The EXTRA_TEST_FILES in root.in was my attempt to add these tests to make fulltest and make cover. There is probably a better way to do this, but I couldn't think of one at the moment.

Well, I'm a bit confused about the current state of config/gen/makefile/root.in in this respect. Once defined EXTRA_TEST_FILES is used only once: 

1599 # extra tests - tests run by make test but not by make fulltest or make cover
1600 extra_tests : test_prep
1601     $(PERL) t/harness $(EXTRA_TEST_ARGS) $(EXTRA_TEST_FILES)

But that comment at line 1599 appears to be contradicted by other, older code in this file. extra_tests is invoked as part of target fulltest: 

1493 fulltest :
[snip]
1503     extra_tests \

... and also (I think) as part of cover: 

2115 cover-extra: cover.dummy
2116     -@make@ extra_tests

I believe that Allison's intent was to create a target which reduced the running time of the most essential tests, without changing people's expectations of what was to be run by make test. Did her revision have a different, unintended result?

Seeking clarification,
kid51

in reply to: ↑ 3   Changed 12 years ago by jkeenan

Replying to jkeenan:

Have created library_files branch to research this.

follow-up: ↓ 6   Changed 12 years ago by mikehh

This was my understanding.

I need to change the comment I added with the EXTRA_TEST_FILES in root.in to reflect the current situation as opposed to the situation before the addition.

What happened is that after the change in r40512 the tests (in EXTRA_TEST_FILES) were no longer being run in make fulltest (also make cover) as they had been moved out of coretests, although they were run in make test and make smoke/smolder_test from library_tests now. It is my understanding that make fulltest should run all the test files.

Additionally make nqp_test was added to make test (BUT NOT make smoke/smolder_test) by moritz. This works slightly differently and also needs looking at.

In terms of lib/Parrot/Harness/DefaultTests.pm the @standard_tests array includes t/doc/*.t which no longer exists.

in reply to: ↑ 5   Changed 12 years ago by jkeenan

Replying to mikehh:

What happened is that after the change in r40512 the tests (in EXTRA_TEST_FILES) were no longer being run in make fulltest (also make cover) as they had been moved out of coretests, although they were run in make test and make smoke/smolder_test from library_tests now. It is my understanding that make fulltest should run all the test files.

So wouldn't it be simpler to have make fulltest start out by running the test target? That way -- setting aside cover issues -- we could dispense with EXTRA_TEST_FILES entirely.

After all, my intuition would be that fulltest would start out with test. I'm surprised to learn that it does not.

  Changed 12 years ago by coke 

On Sun, Sep 27, 2009 at 2:18 PM, Parrot <parrot-tickets@lists.parrot.org> wrote:
> #1061: Lists of test files duplicated.
> ---------------------+------------------------------------------------------
>  Reporter:  coke     |       Owner:  jkeenan
>     Type:  cage     |      Status:  assigned
>  Priority:  minor    |   Milestone:
> Component:  testing  |     Version:  trunk
>  Severity:  medium   |    Keywords:
>     Lang:           |       Patch:
>  Platform:           |
> ---------------------+------------------------------------------------------
>
> Comment(by jkeenan):
>
>  Replying to [comment:5 mikehh]:
>
>  >
>  > What happened is that after the change in r40512 the tests (in
>  EXTRA_TEST_FILES) were no longer being run in make fulltest (also make
>  cover) as they had been moved out of coretests, although they were run in
>  make test and make smoke/smolder_test from library_tests now. It is my
>  understanding that make fulltest should run all the test files.
>  >
>
>  So wouldn't it be simpler to have `make fulltest` start out by running the
>  `test` target?  That way -- setting aside `cover` issues -- we could
>  dispense with `EXTRA_TEST_FILES` entirely.
>
>  After all, my intuition would be that `fulltest` would start out with
>  `test`.  I'm surprised to learn that it does not.
>
> --
> Ticket URL: <https://trac.parrot.org/parrot/ticket/1061#comment:6>
> Parrot <https://trac.parrot.org/parrot/>
> Parrot Development
>

test runs tests under the default core; these are covered in one of
the of the explicit test runs covered by fulltest, so there's no need
to run it 2x.

-- 
Will "Coke" Coleda

in reply to: ↑ description ; follow-up: ↓ 9   Changed 12 years ago by jkeenan

Replying to coke:

This information shouldn't be duplicated in 2 locations. I recommend not using the perl module to specify this via t/harness, but instead to declare the lists solely in the Makefile and then combine them as necessary if chunks of tests files are shared across test invocations.

I have an alternative recommendation. Let's provide access to the definitions in Parrot::Harness::DefaultTests to the Makefile. To do that, I have defined some assignments in config/init/defaults.pm. Those get added to the Parrot::Configure object and are therefore available for assignment to make targets via config/gen/makefiles/root.in.

I have renamed some variables and make targets. I have run make fulltest and demonstrated that what were the EXTRA_TEST_FILES are being run in that target.

mikehh: Can you verify that these work in make cover?

Coke: Is this alternative approach acceptable to you?

Thank you very much.

kid51

Changed 12 years ago by jkeenan

svn diff between trunk (at branch point) and library_files branch

in reply to: ↑ 8   Changed 12 years ago by jkeenan

Replying to jkeenan:

Replying to coke:

I have an alternative recommendation. Let's provide access to the definitions in Parrot::Harness::DefaultTests to the Makefile. To do that, I have defined some assignments in config/init/defaults.pm. Those get added to the Parrot::Configure object and are therefore available for assignment to make targets via config/gen/makefiles/root.in.

Having heard no objections, I plan to merge this into trunk on Tuesday, October 13. So please register any objections now.

Thank you very much.

kid51

follow-up: ↓ 11   Changed 12 years ago by jkeenan

  • patch set to applied

library_files branch was merged into trunk in r41849. Will observe impact, then close ticket in 2-3 days if there are no problems.

Thank you very much.

kid51

in reply to: ↑ 10   Changed 12 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

library_files branch was merged into trunk in r41849. Will observe impact, then close ticket in 2-3 days if there are no problems.

No complaints in the designated period; closing ticket.

Note: See TracTickets for help on using tickets.