1402151 - Convert FTP parser unit tests into a gtest

Status: RESOLVED FIXED

(Core :: Networking: FTP, enhancement, P5)

Description

[Andrew McCreight [:mccr8]]

There are a number of test cases for the FTP directory listing parser in the tree, but I don't think they are run in automation. It won't be that hard to convert them to a gtest.

Comment 1

[Andrew McCreight [:mccr8]]

I've got this basically up and running, but there are two issues I've come across:

- Years are not converted properly in a number of places. The NSPR exploded time format says that the year should be absolute years, but for some reason this code goes out of its way in a few places to store the time since 1980. I think I've fixed it, but the question is if any patches to the parser code are okay.

- In at least one of the formats, the date is given as seconds since whenever, and has to be converted into a regular date. The problem here is that the resulting date depends on the time zone. The current code uses the computer's current time zone, which is obviously not okay for a test that has to run in automation. It isn't hard to parameterize the parsing code by an NSPR timezone callback, but the only preexisting fixed time zone in the NSPR codebase that I see is for GMT. However, the tests seem to have been targeted at a time zone one hour west of GMT, that takes into account DST, so times during the summer months are off by one hour, and during the winter months are off by two hours (or maybe vice versa). I could either write a custom callback that copies all of the tedious calculations for figuring out whether daylight savings time applies depending on the date, or rewrite the tests to use GMT. I guess if I write a script, the latter is probably a little better. I started converting them manually, but that is awful because there are lots of tests.

There are some other minor issues, like there's a test for the Windows parser for <JUNCTION> when it doesn't have a target. The test expects the parser to deal with that, but it just gets thrown out as junk. The Chrome FTP parser doesn't handle JUNCTION at all, so I think the right approach is to just delete that line of the test.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Paris to GMT conversion script

In the end, it turned out that only a single test, E-EPLF.out, had the Paris time zone instead of GMT. That test still requires a little bit of manual fixup beyond this, possibly due to a change or a bug in when daylight savings time kicks in, but I didn't care enough to investigate further. Here's the script I used, for posterity.

Comment 3

[Andrew McCreight [:mccr8]]

try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc3ca8283e4f4937e69703ceb9a6a98cec13bfe

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 1 - Improve handling of two digit years.

The tm_year field of PRExplodedTime is an absolute year, not years
since 1900, so fix a few places that try to do the latter.

I also made the conversion of 2 digit years more consistent.

Review commit: https://reviewboard.mozilla.org/r/184804/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184804/

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 2 - Clamp the number of blocks for a VMS file.

strtoul returns an |unsigned long|, and clamps the return value to
ULONG_MAX if the input is out of range. This means that the value
returned for large numbers differs between 32-bit and 64-bit
systems. To make testing easier, this patch clamps the return value to
UINT32_MAX, ensuring that 32-bit and 64-bit builds of Firefox will be
consistent.

Explicitly declaring that the intermediate value numBlocks is a
uint64_t also avoids (well-defined) integer overflow in the case of a
large file.

This patch also changes PRId64 to PRIu64, because the value being
printed is unsigned. However, this should not make a difference in
practice, because, with clamping, the maximum value being printed is
|UINT32_MAX * 512|.

I also cleaned up the comment that was a little garbled and contained
information about both the old and new ways this was handled, and
removed some code that has been ifdef'd out for at least 14 years.

Review commit: https://reviewboard.mozilla.org/r/184806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184806/

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 3 - Move parse-ftp unit tests into the gtest directory.

This will make them easier to access from a gtest.

Review commit: https://reviewboard.mozilla.org/r/184808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184808/

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 4 - Fix some FTP parser tests.

D-WinNT: The parser treats <JUNCTION> without an arrow as junk for
some reason. Chrome's FTP directory listing parser doesn't seem to
support <JUNCTION> at all, so it can't be too important. I just
deleted that from the expected output to match Firefox's current
behavior. I also added basic tests for three bugs that were fixed
without any tests landed.

E-EPLF.out: Add a license to the top.

U-WinNT.out: We're not testing junk or comment output, and we don't
use the list: prefix, so remove all of those. Also, we now output the
proper year (2000 instead of 00, for instance), so fix that in the
test output. Finally, the time in the output for this test is
formatted differently, for whatever reason, so change it to match the
actual output.

V-VMS-mix.out: Fix file sizes. File sizes on VMS can only be
approximated. When the FTP tests initially landed in 2002, this was
handled by not trying. Bug 22299 landed in 2003 and changed our
behavior to approximate the size, but the tests were not
updated.

Review commit: https://reviewboard.mozilla.org/r/184810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184810/

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 5 - Adjust E-EPLF.out from Paris time to GMT.

The dates in the input for this file are specified as seconds since
Jan 1, 1970 or whenever. The conversion to a normal date depends on
the current time zone. The original output seems to have been
generated using GMT+1, with adjustments for daylight savings time at
the appropriate parts of the year. To make this easier to test, this
patch changes all of the times to GMT, without any daylight savings
time adjustment.

This change was automatically created by the Python script attached to
the bug.

Review commit: https://reviewboard.mozilla.org/r/184812/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184812/

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 6 - Manual fixups for times in E-EPLF.out.

After the automatic fixes in the previous patch, a few of the times in
late March and early April are still off by an hour. Maybe this was
affected by the changes in when daylight savings time or some bug in
my conversion script. I have no idea. This patch just fixes the test
to match the actual output.

Review commit: https://reviewboard.mozilla.org/r/184814/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184814/

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 7 - Add a time zone argument to ParseFTPList.

In a few places, the FTP parser needs to know the time zone and
daylight savings time correction, encapsulated in
PRTimeParamFn. Before this patch, it would use the time zone of the
current machine, which would obviously make automated testing
difficult. This patch allows a caller to pass in a PRTimeParamFn,
which will allow the gtest to use GMT instead of whatever the local
time is.

Review commit: https://reviewboard.mozilla.org/r/184816/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184816/

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 8 - Add an argument to ParseFTPList to return the current time.

If the year is omitted from a listing, then the parsing code assumes
that the date occurred within the last year. To compute what the year
should be, the code thus needs to know what the current year, month
and day are. This patch parameterizes ParseFTPList over a function
that returns a PRTime. This will allow the gtest to pass in a fixed
time. Firefox itself will continue to use PR_Now to get the current
time.

Review commit: https://reviewboard.mozilla.org/r/184818/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184818/

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

This test turns the existing stand alone test for the FTP directory
listing parser into a gtest.

Review commit: https://reviewboard.mozilla.org/r/184820/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/184820/

Comment 13

[Michal Novotny [:michal]]

Comment on attachment 8913472 [details]
Bug 1402151, part 1 - Improve handling of two digit years.

https://reviewboard.mozilla.org/r/184804/#review193340

Comment 14

[Michal Novotny [:michal]]

Comment on attachment 8913473 [details]
Bug 1402151, part 2 - Clamp the number of blocks for a VMS file.

https://reviewboard.mozilla.org/r/184806/#review193354

Comment 15

[Michal Novotny [:michal]]

Comment on attachment 8913474 [details]
Bug 1402151, part 3 - Move parse-ftp unit tests into the gtest directory.

https://reviewboard.mozilla.org/r/184808/#review193356

Comment 16

[Michal Novotny [:michal]]

Comment on attachment 8913475 [details]
Bug 1402151, part 4 - Fix some FTP parser tests.

https://reviewboard.mozilla.org/r/184810/#review193360

Comment 17

[Michal Novotny [:michal]]

Comment on attachment 8913476 [details]
Bug 1402151, part 5 - Adjust E-EPLF.out from Paris time to GMT.

https://reviewboard.mozilla.org/r/184812/#review193366

Comment 18

[Michal Novotny [:michal]]

Comment on attachment 8913477 [details]
Bug 1402151, part 6 - Manual fixups for times in E-EPLF.out.

https://reviewboard.mozilla.org/r/184814/#review193370

Comment 19

[Michal Novotny [:michal]]

Comment on attachment 8913478 [details]
Bug 1402151, part 7 - Add a time zone argument to ParseFTPList.

https://reviewboard.mozilla.org/r/184816/#review193372

Comment 20

[Michal Novotny [:michal]]

Comment on attachment 8913479 [details]
Bug 1402151, part 8 - Add an argument to ParseFTPList to return the current time.

https://reviewboard.mozilla.org/r/184818/#review193374

Comment 21

[Michal Novotny [:michal]]

Comment on attachment 8913480 [details]
Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

https://reviewboard.mozilla.org/r/184820/#review193376

Comment 22

[Andrew McCreight [:mccr8]]

Thanks for the reviews.

Comment 23

[Andrew McCreight [:mccr8]]

Comment on attachment 8913472 [details]
Bug 1402151, part 1 - Improve handling of two digit years.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184804/diff/1-2/

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8913473 [details]
Bug 1402151, part 2 - Clamp the number of blocks for a VMS file.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184806/diff/1-2/

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 8913474 [details]
Bug 1402151, part 3 - Move parse-ftp unit tests into the gtest directory.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184808/diff/1-2/

Comment 26

[Andrew McCreight [:mccr8]]

Comment on attachment 8913475 [details]
Bug 1402151, part 4 - Fix some FTP parser tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184810/diff/1-2/

Comment 27

[Andrew McCreight [:mccr8]]

Comment on attachment 8913476 [details]
Bug 1402151, part 5 - Adjust E-EPLF.out from Paris time to GMT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184812/diff/1-2/

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 8913477 [details]
Bug 1402151, part 6 - Manual fixups for times in E-EPLF.out.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184814/diff/1-2/

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 8913478 [details]
Bug 1402151, part 7 - Add a time zone argument to ParseFTPList.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184816/diff/1-2/

Comment 30

[Andrew McCreight [:mccr8]]

Comment on attachment 8913479 [details]
Bug 1402151, part 8 - Add an argument to ParseFTPList to return the current time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184818/diff/1-2/

Comment 31

[Andrew McCreight [:mccr8]]

Comment on attachment 8913480 [details]
Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/184820/diff/1-2/

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ebb3c0fb804
part 1 - Improve handling of two digit years. r=michal
https://hg.mozilla.org/integration/autoland/rev/1878cd54d8a2
part 2 - Clamp the number of blocks for a VMS file. r=michal
https://hg.mozilla.org/integration/autoland/rev/b866226c10a4
part 3 - Move parse-ftp unit tests into the gtest directory. r=michal
https://hg.mozilla.org/integration/autoland/rev/427f0784d60c
part 4 - Fix some FTP parser tests. r=michal
https://hg.mozilla.org/integration/autoland/rev/f60db8236445
part 5 - Adjust E-EPLF.out from Paris time to GMT. r=michal
https://hg.mozilla.org/integration/autoland/rev/8c4e5c96e95f
part 6 - Manual fixups for times in E-EPLF.out. r=michal
https://hg.mozilla.org/integration/autoland/rev/90904b0eae64
part 7 - Add a time zone argument to ParseFTPList. r=michal
https://hg.mozilla.org/integration/autoland/rev/cef4777d22b6
part 8 - Add an argument to ParseFTPList to return the current time. r=michal
https://hg.mozilla.org/integration/autoland/rev/f013760760f2
part 9 - Implement gtest for FTP directory listing parsing. r=michal

Comment 33

[Code Review Bot [:reviewbot]]

Comment on attachment 8913480 [details]
Bug 1402151, part 9 - Implement gtest for FTP directory listing parsing.

https://reviewboard.mozilla.org/r/184820/#review194494


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:156
(Diff revision 2)
> +  "U-wu",
> +  "V-MultiNet",
> +  "V-VMS-mix",
> +};
> +
> +TEST(ParseFTPTest, Check)

Warning: Use '= default' to define a trivial default constructor [clang-tidy: modernize-use-equals-default]

::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:156
(Diff revision 2)
> +  "U-wu",
> +  "V-MultiNet",
> +  "V-VMS-mix",
> +};
> +
> +TEST(ParseFTPTest, Check)

Warning: Use '= delete' to prohibit calling of a special member function [clang-tidy: modernize-use-equals-delete]

::: netwerk/test/gtest/parse-ftp/TestParseFTPList.cpp:163
(Diff revision 2)
> +  PRStatus result = PR_ParseTimeString(kDefaultTestTime, true, &gTestTime);
> +  ASSERT_EQ(PR_SUCCESS, result);
> +
> +  char inputFileName[200];
> +  char resultFileName[200];
> +  for (size_t test = 0; test < mozilla::ArrayLength(testFiles); ++test) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5ebb3c0fb804
https://hg.mozilla.org/mozilla-central/rev/1878cd54d8a2
https://hg.mozilla.org/mozilla-central/rev/b866226c10a4
https://hg.mozilla.org/mozilla-central/rev/427f0784d60c
https://hg.mozilla.org/mozilla-central/rev/f60db8236445
https://hg.mozilla.org/mozilla-central/rev/8c4e5c96e95f
https://hg.mozilla.org/mozilla-central/rev/90904b0eae64
https://hg.mozilla.org/mozilla-central/rev/cef4777d22b6
https://hg.mozilla.org/mozilla-central/rev/f013760760f2