Skip to content

Conversation

@thatcomputerguy0101
Copy link
Contributor

@thatcomputerguy0101 thatcomputerguy0101 commented Nov 3, 2025

Description

Revision of #2164. Instead of only running headless tests during a build, now only the images are disabled. To reenable showing images, the enableTestUi project property needs to be passed to gradle.

./gradlew test -PenableTestUi

Meta

Merge checklist:

  • Pull Request title is short, imperative summary of proposed changes
  • The description documents the what and why
  • This PR has been linted.
  • If this PR changes behavior or adds a feature, user documentation is updated
  • If this PR touches photon-serde, all messages have been regenerated and hashes have not changed unexpectedly
  • If this PR touches configuration, this is backwards compatible with settings back to v2025.3.2
  • If this PR touches pipeline settings or anything related to data exchange, the frontend typing is updated
  • If this PR addresses a bug, a regression test for it is added

@thatcomputerguy0101 thatcomputerguy0101 requested a review from a team as a code owner November 3, 2025 01:29
@github-actions github-actions bot added the backend Things relating to photon-core and photon-server label Nov 3, 2025
@Gold856
Copy link
Collaborator

Gold856 commented Nov 3, 2025

I'm not sure how I feel about the level of granularity here, or the implementation. It feels like a lot of complexity for little gain. I'd prefer setting and reading environment variables, at the least.

@thatcomputerguy0101
Copy link
Contributor Author

Fair point to the granularity, since individual tests can also be selected. Why use environment variables over system properties? A dedicated task can be created using system properties if the granularity is removed.

@Gold856
Copy link
Collaborator

Gold856 commented Nov 4, 2025

Environment variables are a little less bespoke in my opinion. You should still be able to have a separate test task that uses a different set of environment variables.

@thatcomputerguy0101
Copy link
Contributor Author

A GradleBuild task only seems to support setting project or system properties, not environment variables, so I will be using project properties unless you have another solution.

@Gold856
Copy link
Collaborator

Gold856 commented Nov 4, 2025

Did you read the StackOverflow link I sent?

@thatcomputerguy0101
Copy link
Contributor Author

I meant creating a task that runs the test task after setting an environment variable does not seem to be possible. The post you linked describes how to set an environment variable for the execution of that task, but it does not carry over into other tasks. Are you implying that the contents of the test task should be duplicated?

@Gold856
Copy link
Collaborator

Gold856 commented Nov 4, 2025

Actually, I just realized even environment variables aren't necessary. You can just use systemProperty("java.awt.headless", !project.hasProperty("enableTestUi")) in the test block and that'll be sufficient to control if tests should be run as headless or not, no other modifications needed.

For environment variables, I was envisioning that you just set that for the test task only, similar to how java.awt.headless is set for testHeadless. I'm not sure why you'd want that environment to carry over to other tasks.

@thatcomputerguy0101
Copy link
Contributor Author

The current implementation of test actually runs all of the headless tests twice, once from test and once from testHeadless. It might be worth only having the test task and parametrizing both the java.awt.headless system property and the exclusion of benchmarks so that headless + benchmarks is the default, advanced debugging can disable headless, and the runner can disable benchmarks. (Although the benchmarks are slow, so maybe they are disabled by default too?)

@Gold856
Copy link
Collaborator

Gold856 commented Nov 5, 2025

Yeah, I’d like to delete testHeadless and just go with the system property controlled by a project property thing I sent. We can just leave the benchmarks as they are in my opinion.

@thatcomputerguy0101
Copy link
Contributor Author

The build-grade CI task currently runs testHeadless, so the benchmarks will need to be parameterized if the same behavior is kept.

@Gold856
Copy link
Collaborator

Gold856 commented Nov 5, 2025

May as well just run the benchmarks.

@github-actions github-actions bot removed the backend Things relating to photon-core and photon-server label Nov 5, 2025
@thatcomputerguy0101
Copy link
Contributor Author

What I pushed now just gives the option for test UI. I found it to be fairly simple to add an option for benchmarks, so I can add that too if the CI run time is bad.

Gold856
Gold856 previously approved these changes Nov 5, 2025
Copy link
Collaborator

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beautiful.

@Gold856
Copy link
Collaborator

Gold856 commented Nov 5, 2025

Two more things actually: we should document this in the build instructions and the PR description should be updated. Then I'll merge.

@github-actions github-actions bot added the documentation Anything relating to https://docs.photonvision.org label Nov 6, 2025
Copy link
Collaborator

@Gold856 Gold856 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gold856 Gold856 enabled auto-merge (squash) November 6, 2025 19:27
@Gold856 Gold856 merged commit def3b9f into PhotonVision:main Nov 6, 2025
41 checks passed
spacey-sooty pushed a commit to spacey-sooty/photonvision that referenced this pull request Dec 22, 2025
…otonVision#2177)

## Description

Revision of PhotonVision#2164. Instead of only running headless tests during a
build, now only the images are disabled. To reenable showing images, the
`enableTestUi` project property needs to be passed to gradle.

```bash
./gradlew test -PenableTestUi
```

## Meta

Merge checklist:
- [X] Pull Request title is [short, imperative
summary](https://cbea.ms/git-commit/) of proposed changes
- [X] The description documents the _what_ and _why_
- [X] This PR has been
[linted](https://docs.photonvision.org/en/latest/docs/contributing/linting.html).
- [x] If this PR changes behavior or adds a feature, user documentation
is updated
- [ ] If this PR touches photon-serde, all messages have been
regenerated and hashes have not changed unexpectedly
- [ ] If this PR touches configuration, this is backwards compatible
with settings back to v2025.3.2
- [ ] If this PR touches pipeline settings or anything related to data
exchange, the frontend typing is updated
- [ ] If this PR addresses a bug, a regression test for it is added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Anything relating to https://docs.photonvision.org

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants