Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make Command tests use command factories and decorators. #7006

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

Make Command tests use command factories and decorators. #7006

wants to merge 48 commits into from
+321 −336

Conversation

TheComputer314
Copy link

@TheComputer314 TheComputer314 commented Aug 25, 2024 •
edited
Loading

Closes #6652.

@TheComputer314 TheComputer314 requested a review from a team as a code owner August 25, 2024 15:47
@github-actions GitHub Actions
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@TheComputer314
Copy link
Author

/format

spacey-sooty
spacey-sooty suggested changes Sep 13, 2024
View reviewed changes
Copy link
Contributor

@spacey-sooty spacey-sooty left a comment

Choose a reason for hiding this comment

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

Looks good to me but needs C++ and Python ports

@TheComputer314 TheComputer314 marked this pull request as draft September 22, 2024 04:22
@TheComputer314
Copy link
Author

/format

@github-actions github-actions bot added the component: command-based WPILib Command Based Library label Dec 8, 2024
KangarooKoala
KangarooKoala reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

I'm a bit iffy about using factories instead of the command constructors in tests that are specifically testing those command types, but the command constructors also don't support CommandPtr. I could see either way (replacing command constructors in command type tests with factories or not using any factories in command type tests) being fine. @rzblue thoughts?

wpilibNewCommands/src/test/native/cpp/frc2/command/DefaultCommandTest.cpp Outdated Show resolved Hide resolved
KangarooKoala
KangarooKoala reviewed Dec 9, 2024
View reviewed changes
Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

Submitted before I meant to, whoops. I've also updated my comment with a list of what needs to be changed.

wpilibNewCommands/src/test/native/cpp/frc2/command/SequentialCommandGroupTest.cpp Outdated Show resolved Hide resolved
@TheComputer314
Copy link
Author

ScheduleCommand doesn't have a factory yet, and I'm not entirely sure how to POVButtonTest.

@TheComputer314
Copy link
Author

/format

@KangarooKoala
Copy link
Contributor

Great progress! I've updated the list of changes to make.

ScheduleCommand doesn't have a factory yet, and I'm not entirely sure how to POVButtonTest.

For ScheduleCommandTest, you just need to replace InstantCommand, and for POVButtonTest, you just need to replace WaitUntilCommand.

P.S. I get email notifications for GitHub comments on this PR (and other PRs I've participated in), and I also usually see all updated PRs (which includes PRs with new commits) within a day. If you want to make sure I see it, though, you can always keep on requesting my review- Emails are cheap!

@TheComputer314
Copy link
Author

For ScheduleCommandTest, ScheduleCommand doesn't have a constructor that accepts a CommandPtr, and for POVButtonTest, (as far as my understand of cpp goes) Trigger only has a constructor for an rvalue CommandPtr when command needs to be used elsewhere, necessitating making it an lvalue.

@KangarooKoala
Copy link
Contributor

Ah, you're right, I missed that. Don't worry about them, then.

@TheComputer314
Copy link
Author

CommandPtr also doesn't have .addRequirements() necessary for AddRequirementsTest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacey-sooty spacey-sooty spacey-sooty requested changes

@KangarooKoala KangarooKoala Awaiting requested review from KangarooKoala

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
component: command-based WPILib Command Based Library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[cmd] Use command factories and decorators in command tests
4 participants
@TheComputer314 @rzblue @KangarooKoala @spacey-sooty