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.

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

Review and update these chunking unit tests #443

Closed
scijones opened this issue Mar 26, 2024 · 4 comments · Fixed by #442
Closed

Review and update these chunking unit tests #443

scijones opened this issue Mar 26, 2024 · 4 comments · Fixed by #442

Comments

@scijones
Copy link
Contributor

For these tests, it's not trivial to say what the correct chunks are with a quick glance. they need to be reviewed in light of the change to make the setting "automatically-create-singletons" "on" by default.

Teach_Soar_90_Games
Demo_ToH_Recursive (probably worth just omitting this test from usual unit testing or rewriting the agent for this test)
Operator_Selection_Knowledge (AKA Operator_Selection_Knowledge_Mega_Test)
Constraint_Prop_from_Base_Conds

Currently, those tests simply use the command "chunk automatically-create-singletons off" to make them work (and in the case of 90 games, even that doesn't work, but that test was also already left with a comment that it wasn't working properly before my changes to create this new command.)

@garfieldnate
Copy link
Collaborator

Do you think that these chunks were actually reviewed in the first place? For tests that generate large sets of data, it's fairly common to just say "what we have right now is probably fine" and then add the test simply to detect unintended changes.

@scijones
Copy link
Contributor Author

scijones commented Mar 28, 2024

They probably weren't reviewed. But, for example, that means I want someone who knows what the 90 games agent should do to verify that it still does the right thing. Then, we could just again store the chunks and say "what we have right now is probably fine" again. I'm just not that guy.

@garfieldnate
Copy link
Collaborator

The guy would be @jrkirk, right?

@scijones
Copy link
Contributor Author

Yes.

@scijones scijones linked a pull request Apr 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants