Migrate current tests to Test2 #702

duffee · 2022-07-22T06:18:12Z

Squashed commits after messing up the rebase onto the latest develop branch for #692 .
Recreated the PR and these are the commit messages rescued from the original branch.

tableau.t
PITA! The circular references in MathObjects made
the deep comparisons impossible, so all MathObjects
were stringified for comparisons.

WARN and DEBUG message sub redefinitions were removed.
A "validator" and a stringify sub were added to get around
some of the comparison issues.

Organized into subtests.

math_objects_basics.t
Convert throws_ok to the like/dies construct
and the tests pass.
Arranged sections in subtests

Three test files migrated from Test::More to Test2 without modification
(other than moving the environment setup from the script to Test::PG)

This expands on the original t/README.md on the details of testing.

t/context
Minor changes to migrate tests in t/context from Test::More to Test2
There were few tests in these files.

toltype_digits.t organized into subtests by topic

Use the Test::PG environment with Test2

Find that the original test produced unexpected results
and the tests that expose the incorrect behaviour have
been wrapped in a TODO block

Added unit tests for Units.pm
Tests to validate the correctness of #677
Unit tests for Units.pm electron volts
Tests for parserNumberWithUnits.pl with basic energy conversions

Adding a Test::PG module in t/lib/ to take the place of t/build_PG_envir.pl

Squashed commits after messing up the rebase onto the latest develop branch. Recreated and these are the commit messages rescued from that. tableau.t PITA! The circular references in MathObjects made the deep comparisons impossible, so all MathObjects were stringified for comparisons. WARN and DEBUG message sub redefinitions were removed. A "validator" and a stringify sub were added to get around some of the comparison issues. Organized into subtests. math_objects_basics.t Convert `throws_ok` to the like/dies construct and the tests pass. Arranged sections in subtests Three test files migrated from Test::More to Test2 without modification (other than moving the environment setup from the script to Test::PG) This expands on the original t/README.md on the details of testing. t/context Minor changes to migrate tests in t/context from Test::More to Test2 There were few tests in these files. toltype_digits.t organized into subtests by topic Use the Test::PG environment with Test2 Find that the original test produced unexpected results and the tests that expose the incorrect behaviour have been wrapped in a TODO block Added unit tests for Units.pm Tests to validate the correctness of openwebwork#677 Unit tests for Units.pm electron volts Tests for parserNumberWithUnits.pl with basic energy conversions Adding a Test::PG module in t/lib/ to take the place of t/build_PG_envir.pl

Convert math_objects/factorial.t to using the Test2 framework Update the t/README.md to the new usage of Test2 and the boilerplate Test::PG module removed the execute permission from tableau.t

duffee · 2022-07-22T09:55:26Z

@pstaabp I've removed the check for PG-2.17 in t/units/electron_volts.t, updated t/README.md and your new test t/math_objects/factorial.t to use Test2. The whole test suite is now consistent and mostly organized.

The only things that I can't seem to fix are the repeated calls to check_score in units/basic_parser.t and cos(90) the todo block at the end of contexts/trig_degrees.t. Any help would be welcome.

pstaabp · 2022-07-22T13:18:50Z

@duffee Now that 2.17 is official, we'll take a look at this for develop. I'll see if I can figure out the check_score issue.

drgrice1

This pull request needs some work. See my comments.

All tests in t/macros pass on the develop branch, but many are failing with this pull request. That will also need to be fixed before this can be merged.

drgrice1 · 2022-08-12T00:19:26Z

t/README.md

+
+
+# Unit Tests
+


I am seeing a couple of markdown lint errors here. First, no multiple blank lines, and second, multiple top level headings in the same file.

drgrice1 · 2022-08-12T00:20:19Z

t/README.md

+
+
+# Integration tests
+


Same errors here as before.

drgrice1 · 2022-08-12T00:20:39Z

t/README.md

+
+
+# Test Dependencies


drgrice1 · 2022-08-12T00:34:26Z

t/contexts/fraction.t


-package main;
+# should I "use" Parser Value Parser::Legacy here instead?


Delete this comment. It is certainly not in the correct place in the code, and it not clear what this comment means.

drgrice1 · 2022-08-12T00:40:15Z

t/lib/Test/PG.pm

+The reason for the module is that There Is More Than One Way To Do It
+and people develop different styles.  This is my coding style.
+Also we learn the underlying structures better by re-inventing the wheel.
+We just try to make a better wheel, but success is not guaranteed.
+
+This module strives for elegance in reducing boiler plate in test files,
+a minimum of duplicated code and adheres to the principle of least surprise
+by locating modules below the C<t/lib> directory.
+It does not make a decent cup of tea.


Please delete these extraneous comments in the POD. Please do not use the first person in comments and please do not comment on "your coding style". POD should be informative as to what the module does and how to use it. It is not a narrative.

"There Is More Than One Way To Do It" and there are lots of incorrect ways to do it. The old perl slogan is hackish nowadays.

drgrice1 · 2022-08-12T01:16:35Z

t/contexts/fraction.t

-use lib "$main::pg_dir/lib";
-
-require("$main::pg_dir/t/build_PG_envir.pl");
+To test the reduction of fractions


Please reword this as something like

Fraction context tests

I know the only test in here was labeled as testing reduce fractions. However, not only is that not what it is testing (it is testing that fractions with different representations are equal -- take note @pstaabp), but in the future this will have more tests that test things with fractions other than just reduction (maybe actually one of those too!).

Also, the first header in the POD should be head1.

drgrice1 · 2022-08-12T01:18:43Z

t/contexts/toltype_digits.t


-# The following needs to include at the top of any testing down to END OF TOP_MATERIAL.
+To test for tolerances


At least remove "To" from the beginning of this. Make it say just "Test tolerances". Either make it a proper statement or make it a complete sentence. "To test for tolerances" do what?

drgrice1 · 2022-08-12T01:28:22Z

t/units/basic_parser.t

+We test the basic functionality of the NumberWithUnits parser,
+F<macros/parserNumberWithUnits.pl>, to give us faith
+that the parser and its methods are working.
+Other test files will probe deeper into specific use cases of
+the NumberWithUnits macro and the L<Units> module.


Please remove "we". Pronoun usage in POD and comments is bad. It can all be said without "we" and "I" and "you", and said much better. For example, reword this as

Test the basic functionality of the NumberWithUnits parser F<...>, to demonstrate that the parser and its methods are working.

No need to reference other test files, and what they will do. You don't know that is true yet.

drgrice1 · 2022-08-12T01:30:07Z

t/units/basic_parser.t

+=head3 Setup
+
+All the boilerplate code is loaded with Test::PG and assume that people run
+it from the root directory with C<prove -l>.
+Load your base modules before loading the macros which depend on them
+and set the Context, if appropriate.
+
+See the example in the documentation of L<Test::PG>
+
+  perldoc t/lib/Test/PG.pm


Please remove this section. There is no need to explain how to use the Test::PG module in every test file. This explanation should be in none of the test files, otherwise it will be copied and will end up in many of them. It is the purpose of the POD in the Test::PG file to explain this.

drgrice1 · 2022-08-12T01:45:50Z

t/units/basic_parser.t

+                           degC => 0, degF => 0, degK => 0,
+            },
+            isValue => T(),
+            context => check_isa 'Parser::Context',


There is a syntax error here. Parentheses are needed around 'Parser::Context' in this situation as follows:

Suggested change

context => check_isa 'Parser::Context',

context => check_isa('Parser::Context'),

Even without the syntax error this test is failing. It needs to be fixed.

t/lib/Test/PG.pm

drgrice1 · 2022-08-19T15:55:39Z

t/macros/tableau.t

+	ok($A == $B, 'test zeroLevel tolerance with ok');
+
+	my $real_object = object {
+		prop isa => 'Value::Real';


I realized that the reason some of these tests are failing is because you used isa. isa is not in the version of Test2::Suite that is included in the Ubuntu repositories (version 0.000129).

In any case, this should be using blessed and not isa as that is a more specific and accurate test (and is in version 0.000129). isa only tells you if the given object is an instance of the given class (any class derived from the given class will pass the test). However, blessed tells you more. It tells you that the object is specifically blessed as that class, not just derived from.

drgrice1

After looking at this closer I see that the lib/Test/PG.pm module that was added here should be dropped, and the method in build_PG_envir.pl should be used instead. The problem is that lib/Test/PG.pm module is not a proper module, and is a hack. The file declares the Test::PG package, and then improperly switches to the main namespace without actually defining anything that is worthy of being called a module in the Test::PG namespace.

The build_PG_envir.pl needs some work too, but it is closer to being correct. The main things that it does incorrectly is that it uses require to load PG.pl and the tests use require to load it. Both of those should be done with do. There are some other details that should be fixed with it as well though.

I have a branch built on top of #709 that does everything I have mentioned here, and gets the tests into order.

drgrice1 · 2022-09-05T21:20:14Z

t/units/basic_parser.t

+             add_fundamental_unit add_unit string TeX / ],
+        'Can we NumberWithUnits';
+
+    ok my $evaluator = $joule->cmp($Nm), 'Get an AnswerEvaluator';


There is another problem here. The cmp method does not take a single argument like this. It only takes hash key value pairs. I think you might be confusing the cmp method with the evaluate method of the AnwerHash class.

drdrew42 · 2022-10-12T16:57:31Z

Closed in favor of #726

duffee added 2 commits July 21, 2022 22:35

Migrate factorial.t to Test2

6896972

Convert math_objects/factorial.t to using the Test2 framework Update the t/README.md to the new usage of Test2 and the boilerplate Test::PG module removed the execute permission from tableau.t

drgrice1 requested changes Aug 12, 2022

View reviewed changes

drgrice1 reviewed Aug 13, 2022

View reviewed changes

t/lib/Test/PG.pm Show resolved Hide resolved

drgrice1 reviewed Aug 19, 2022

View reviewed changes

drgrice1 mentioned this pull request Aug 20, 2022

Move the WeBWorK::PG module to PG and improve PG's local configuration. #709

Merged

drgrice1 reviewed Aug 20, 2022

View reviewed changes

This was referenced Aug 20, 2022

Migration to test2 fixed duffee/pg#1

Closed

Add a cpanfile for the PG repo #691

Merged

drgrice1 reviewed Sep 5, 2022

View reviewed changes

duffee mentioned this pull request Sep 8, 2022

Delete duplicate declaration of $deg in contextTrigDegrees #717

Closed

drgrice1 mentioned this pull request Sep 13, 2022

Update the unit tests to use a full PG environment #726

Merged

drdrew42 closed this Oct 12, 2022


		package main;
		# should I "use" Parser Value Parser::Legacy here instead?


		# The following needs to include at the top of any testing down to END OF TOP_MATERIAL.
		To test for tolerances

	context => check_isa 'Parser::Context',
	context => check_isa('Parser::Context'),

Uh oh!

Migrate current tests to Test2 #702

Migrate current tests to Test2 #702

Uh oh!

Conversation

duffee commented Jul 22, 2022

Uh oh!

duffee commented Jul 22, 2022

Uh oh!

pstaabp commented Jul 22, 2022

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drgrice1 Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drgrice1 Aug 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drgrice1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drdrew42 commented Oct 12, 2022

Uh oh!

Uh oh!

drgrice1 Aug 12, 2022 •

edited

Loading

drgrice1 Aug 12, 2022 •

edited

Loading