t/porting/authors.t: failing on many smoke-testing rigs

[jkeenan]

In the past few days quite a few of our smoke-testing rigs have been reporting failures in t/porting/authors.t. See, for example, this report on darwin.

The logfile corresponding to the above report holds repeated instances of this:

Warning: -L./mylib changed to -L/Users/brian/.perl-smoke/perl-current/cpan/ExtUtils-MakeMaker/t/FJMJE_LJvb/./mylib
#   Failed test 'Uncommitted changes are by a known contributor?'
#   at Porting/updateAUTHORS.pl line 156.
#          got: '0'
#     expected: '1'
# 
# There are uncommitted changes in the working directory
# ?? mktest.out
# 
# and your git credentials are new to us. We think that git thinks your
# credentials are as follows (git may use defaults we don't guess
# properly):
# 
#     Name = brian d foy
#     Email = briandfoy@pobox.com
# 
# You have configured the following relevant git config settings:
# 
#     user.name        = brian d foy
#     user.email       = briandfoy@pobox.com
# 
# 
# To resolve this you can perform one or more of these steps:
# 
#     1. Remove the uncommitted changes, including untracked files that
#        show up in
# 
#             git status
# 
#        if you wish to REMOVE UNTRACKED FILES and DELETE ANY CHANGES
#        you can
# 
#             git clean -dfx
#             git checkout -f
# 
#         BE WARNED: THIS MAY LOSE DATA.
# 
#     2. You are already configured in git and you just need to add
#        yourself to AUTHORS and other infra: commit the changes in the
#        working directory, including any untracked files that you plan to
#        add (the rest should be removed), and then run
# 
#             Porting/updateAUTHORS.pl
# 
#        to update the AUTHORS and .mailmap files automatically. Inspect
#        the changes it makes and then commit them once you are
#        satisfied. This is your option to decide who you will be known
#        as in the future!
# 
#     3. You are already a contributor to the project but you are committing
#        changes on behalf of someone who is new. Run
# 
#             Porting/updateAUTHORS.pl
# 
#        to update the AUTHORS and .mailmap files automatically. Inspect
#        the changes it makes and then commit them once you are satisfied.
#        Make sure the conributor is ok with the decisions you make before
#        you merge.
# 
#     3. You are already an author but your git config is broken or
#        different from what you expect, or you are a new author but you
#        havent configured your git details properly, in which case you
#        can use something like the following commands:
# 
#             git config --set user.name "Some Name"
#             git config --set user.email "somewhere@provider"
# 
#        If you are known to the project already this is all you need to
#        do. If you are not then you should perform option 2 or 4 as well
#        afterwards.
# 
#     4. You do not want to be listed in AUTHORS: commit the changes,
#        including any untracked unignored files, and then run
# 
#             Porting/updateAUTHORS.pl --exclude
# 
#        and commit the changes it creates. This test should pass once
#        those commits are created. Thank you for your contributions.
# Looks like you failed 1 test of 7.
[2022-08-23 07:00:36-0400] t/porting/authors ................................................ FAILED at test 7

The earliest commit at which we can see these failures (at least at Tony's aggregator) is 015aea7:

commit 015aea7e08074957eba04c7e3dd19f3a1c1c3738
Author:     Yves Orton <demerphq@gmail.com>
AuthorDate: Sat Aug 20 19:20:30 2022 +0200
Commit:     Yves Orton <demerphq@gmail.com>
CommitDate: Sun Aug 21 12:09:05 2022 +0200

    updateAUTHORS.p[lm] - add testing for dupe names and emails in AUTHORS

I am not getting these failures on my smoke-testing rigs, but my smoke-test runs are kicked-off manually. My impression is that the rigs that are showing test failures run unattended off of cron jobs.

Does anyone have suggestions as to how we can get these smoke-testing rigs running smoothly again?

[demerphq]

We have a temporary work around. But imo it would be better to check for an env flag that a smoke test is running and disable the pending authors tests for those environments. @Tux is there such an env var? Eg, when the smokes run do you set "SMOKE_TEST_RUNNING" or something like that?

[jkeenan]

#20142 (commit 6c19d6c) appears to be stanching the bleeding. See, e.g., https://perl5.test-smoke.org/report/5021934, which was run at a commit after the p.r. was applied. I'll keep an eye on this over the next several days. (I don't yet have a firm opinion on what a more permanent fix should do.)

[Leont]

We have a temporary work around. But imo it would be better to check for an env flag that a smoke test is running and disable the pending authors tests for those environments. @Tux is there such an env var? Eg, when the smokes run do you set "SMOKE_TEST_RUNNING" or something like that?

Per Lancaster Consensus, AUTOMATED_TESTING is the designated environmental variable for such a thing

[demerphq]

Thanks @Leont. SInce I wasn't involved in this how would i get a point of concern registered for the next "consensus": one problem we are facing is that smoke testers are modifying the tree as part of their tests. Its not clear to me if AUTOMATED_TESTING is the right flag to signal "in this environment the code might be modified and the tests should not care".

It does look that until there is a flag that captures that intent that AUTOMATED_TESTING is the closest we can get.

[bram-perl]

git status on my Windows smoker:

$ git status
On branch blead
Your branch is up to date with 'origin/blead'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        modified:   patchlevel.h

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        dist/IO/sock-4896
        dist/IO/sock-9416
        mktest.jsn
        mktest.out
        mktest.rpt
        win32/smoke.mk

no changes added to commit (use "git add" and/or "git commit -a")

So there are several issues:

• untracked mktest.* files
• untracked win32/smoke.mk
• tests that failed to cleanup (but PR io_unix test leaves stray tmp file #20179 might fix that - I did not yet verify the cause nor the fix)

And then there is the modification of patchlevel.h..
On my Windows smoker the only change is changing the line-endings from LF to CRLF.. (I'm not sure if that is also the only change during the build... reading the code suggests it should be adding a registered patch in the form of SMOKE....)

But that last thing got me thinking...
Test::Smoke has support for:

• applying patches: if code is patched then it - obviously - leaves the tree in a dirty state
• skipping tests: and it does this by either modifying the file in t/... or by removing the test file from MANIFEST

So in both cases it will be running the tests with a dirty tree and will lead to failures...

So I guess that makes the question: how can we skip the 'uncommitted changes' part of the test?
Test::Smoke does not appear to be setting any ENV vars...
One could be added but then it may take some time for all smokers to update (I've got no idea how long or even if there is an easy way to contact them all)..

A possibly (ugly?) change could be something like (untested!):

         my $uncommitted_files= $self->git_status_porcelain;
         if ($uncommitted_files) {
-            my ($author_name, $author_email)=
-                $self->current_author_name_email();
-            my ($committer_name, $committer_email)=
-                $self->current_committer_name_email();
+            SKIP: {
+                if ($ENV{"AUTOMATED_TESTING"}) { skip "Skipping because automated testing is set" }
+                if (-f "mktest.out") { skip "Skipping because Test::Smoke appears to be testing"; }
+
+                my ($author_name, $author_email)=
+                    $self->current_author_name_email();
+                my ($committer_name, $committer_email)=
+                    $self->current_committer_name_email();
                   ......

(Another check - instead of mktest.out could be to check the patchlevel but I'm not entirely sure that the code changing the patchlevel.h is still working as expected..)

Any thoughts?

[demerphq]

On Mon, 29 Aug 2022 at 22:04, Bram ***@***.***> wrote: + if (-f "mktest.out") { skip "Skipping because Test::Smoke appears to be testing"; }

I like the idea of this, but woudlnt mktest.jsn be a better choice? I am assuming the .jsn file will be written to disk first before mktest.out. It just feels a bit chicken and egg to use the output of the tests to control the tests... Yves

…

-- perl -Mre=debug -e "/just|another|perl|hacker/"

[bram-perl]

• if (-f "mktest.out") { skip "Skipping because Test::Smoke appears to be testing"; }

I like the idea of this, but woudlnt mktest.jsn be a better choice? I am assuming the .jsn file will be written to disk first before mktest.out. It just feels a bit chicken and egg to use the output of the tests to control the tests...

If I remember it correctly: (but I'll double check later)

• mktest.out is the first one written to
• mktest.jsn is an archived version of the information send to the smoke-database by the reporter; i.e. it's one of the last things it does after running all the build configurations

Also: don't confuse mktest.out for the output of make test, it contains much more then that. (and exists before make test is ran)

[demerphq]

mktest.out is the first one written to

Darn. :-) Ok, well then i guess it has to be mktest.out. :-)

[jkeenan]

Since the problem I originally reported in this ticket has been cleared up for 10 days, and since subsequent discussion has veered into a discussion of smoke-testing which will inevitably require discussion with the Test-Smoke maintainers, I'd like to recommend closing this ticket. If someone wants to open up a ticket in Test-Smoke's queue or start a discussion on our mailing list, that would be fine.

Self-assigning this ticket for the purpose of closing it within 7 days.

[jkeenan]

Since the problem I originally reported in this ticket has been cleared up for 10 days, and since subsequent discussion has veered into a discussion of smoke-testing which will inevitably require discussion with the Test-Smoke maintainers, I'd like to recommend closing this ticket. If someone wants to open up a ticket in Test-Smoke's queue or start a discussion on our mailing list, that would be fine.

Self-assigning this ticket for the purpose of closing it within 7 days.

Closing as per schedule.