Summary

Make Perforce tests use a local p4d.

Review Request #14546 — Created Aug. 1, 2025 and submitted Sept. 4, 2025, 5:09 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

Very early on, we wrote unit tests for the perforce implementation that
connected to public.perforce.com:1666. This worked okay for most things,
but it always felt kind of wrong.

This change makes it so we'll instead spin up a local p4d. This works
for almost all of our tests, and the slight delay I had to add during
that spin-up process is offset by no longer having to make actual
requests to the internet.

It seems like something in the Perforce tests was actually making a lot
of the Perforce tests run successfully even if they were broken. I'm not
sure which of my changes uncovered it, but there were several tests for
revision parsing which were obsolete (since revision parsing moved
completely into the diff parser). I've just removed those tests.

reviewboard/scmtools/testdata/p4_repo is part of the commit but has
been excluded from the posted change.

Testing Done

Ran all perforce tests and saw that all of them actually ran, and that
the previously broken stunnel tests now passed.

Commits

Summary	ID
Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	a4e95cec8c3c5be0e97ba78393477fa2a6158c95

Summary

Make Perforce tests use a local p4d.

Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.

a4e95cec8c3c5be0e97ba78393477fa2a6158c95

Issues

Description	From	Last Updated
too many blank lines (2) Column: 5 Error code: E303	reviewbot	Aug. 1, 2025, 10:45 a.m.
I think what you can do here is to update the if above to do: if P4 is not None …	chipx86	Aug. 8, 2025, 11:20 a.m.
Worried this is going to be an issue on the CI server, which has a tendency to trip up timing-related …	chipx86	Aug. 8, 2025, 11:20 a.m.
Leftover debug statements.	chipx86	Aug. 8, 2025, 11:20 a.m.
Might be worth seeing how much work it'd be to remote-control something for this, but maybe not worth it. Though …	chipx86	Aug. 8, 2025, 10:54 a.m.
Can we pick a different port? 1666 being the default port will conflict with actual local usage. How about something …	chipx86	Aug. 21, 2025, 8:04 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/scmtools/tests/test_perforce.py (Diff revision 1)
The issue has been dropped. Show all issues
```
too many blank lines (2)

Column: 5
Error code: E303
```

Commits:

	Summary	ID
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	91ce93f4f4b7eb53c084da38f08cce1830b19e25
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	7b8fe33e898d7054e44e9ad9a6fd5f6baacd114c

Diff:

Revision 2 (+508 -526)

Show changes

	.gitignore
	reviewboard/scmtools/tests/test_perforce.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
You're awesome.
```

reviewboard/scmtools/tests/test_perforce.py (Diff revision 2)

The issue has been dropped. Show all issues

I think what you can do here is to update the if above to do:

if P4 is not None or TYPE_CHECKING:
    ...

That should it from even considering the possibility of this code path.

david Aug. 8, 2025, 11:23 a.m.

That hits the same problem with unreachable code detection as we've previously discussed.

reviewboard/scmtools/tests/test_perforce.py (Diff revision 2)

The issue has been resolved. Show all issues

Worried this is going to be an issue on the CI server, which has a tendency to trip up timing-related things like this.

Instead (or, along with), can we have it try N times to perform some query and then sleep 0.1 between them?

That'll also be a good place to provide a suitable error if we can't do that connection, so it doesn't fall into the tests.

reviewboard/scmtools/tests/test_perforce.py (Diff revision 2)

The issue has been resolved. Show all issues

Leftover debug statements.

reviewboard/scmtools/tests/test_perforce.py (Diff revision 2)

The issue has been resolved. Show all issues

Might be worth seeing how much work it'd be to remote-control something for this, but maybe not worth it.

Though we should be testing auth failures. Don't we do that? I might be misunderstanding.

Change Summary:


Changed it so we run p4d for the lifetime of the test case class.
Improved invocation to spin until we successfully run p4 info instead of just sleeping for 0.2s
Added a password-protected user so we can have the auth failure test.

Commits:

	Summary	ID
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	7b8fe33e898d7054e44e9ad9a6fd5f6baacd114c
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	db050648b118fbc9c624476365973dc269c5cec0

Diff:

Revision 3 (+514 -468)

Show changes

	.gitignore
	reviewboard/scmtools/tests/test_perforce.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/scmtools/tests/test_perforce.py (Diff revision 3)

The issue has been resolved. Show all issues

Can we pick a different port? 1666 being the default port will conflict with actual local usage. How about something higher like 61666?

Commits:

	Summary	ID
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	db050648b118fbc9c624476365973dc269c5cec0
	Make Perforce tests use a local p4d. Very early on, we wrote unit tests for the perforce implementation that connected to public.perforce.com:1666. This worked okay for most things, but it always felt kind of wrong. This change makes it so we'll instead spin up a local p4d. This works for almost all of our tests, and the slight delay I had to add during that spin-up process is offset by no longer having to make actual requests to the internet. It seems like something in the Perforce tests was actually making a lot of the Perforce tests run successfully even if they were broken. I'm not sure which of my changes uncovered it, but there were several tests for revision parsing which were obsolete (since revision parsing moved completely into the diff parser). I've just removed those tests. Testing Done: Ran all perforce tests and saw that all of them actually ran, and that the previously broken stunnel tests now passed.	a4e95cec8c3c5be0e97ba78393477fa2a6158c95

Diff:

Revision 4 (+514 -468)

Show changes

	.gitignore
	reviewboard/scmtools/tests/test_perforce.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-7.x (085da83)