Differences

This shows you the differences between two versions of the page.

--- qa:preventing_regressions [2025/07/08 23:15] – [C logic] sandbergja
+++ qa:preventing_regressions [2025/07/30 09:35] (current) – [IDL problems] sandbergja
@@ Line 1: / Line 1: @@
 ====== Preventing regressions ======
-You've fixed a bug in Evergreen!  That's great!  Let's prevent it from happening again in the future.  After all, most bugs happen because they are perfectly reasonable mistakes to make.  Including one or more of the following techniques when you submit your bug fix can provide a safety net for future contributors to make sure they don't inadvertently damage your contribution.
+You've fixed a bug in Evergreen!  That's great!
+However, there's a dark side.  Most bugs happen because they are perfectly reasonable mistakes to make.  Therefore, another perfectly reasonable contributor, not knowing about the history of your bug fix, might some day re-introduce the exact same bug that you just fixed.
+How can we help make sure that your bug never rears its ugly head again?  Including one or more of the following techniques when you submit your bug fix can provide a safety net for future contributors that keeps them from introducing a regression.
 ===== Angular component templates =====
-If the bug was something specific to this component, for example, it displayed its data incorrectly or a user interaction was broken: an angular unit test can be useful.
+If the bug was something specific to this component, for example, it displayed its data incorrectly, had some incorrect display logic, or a user interaction was broken: an angular unit test can be useful.
-If the bug was a general bad practice that could happen in any component (for example, important @Inputs not marked for translation, CSS classes combined that don't look good together, broken accessibility semantics), try writing a custom ESLint rule.
+If the bug was a general bad practice that could happen in any component (for example, important ''@Input''s not marked for translation, CSS classes combined that don't look good together, broken accessibility semantics), try writing a custom ESLint rule.  These will check all templates that don't specifically opt out, so it's a good way to do these checks at scale.
+If the bug was a general accessibility bug (e.g. form inputs not associated with their labels), another nice option is a nightwatch end-to-end (e2e) test.  Nightwatch can load your page in a browser then [[https://nightwatchjs.org/guide/using-nightwatch/accessibility-testing.html|run aXe automated checks against it]].
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/1a3e59375f408b4aa99d95e320d0810026668412|Example tests that make assertions about an addition to a template]] (although this is in the context of a new Angular feature rather than a bug fix)
+  * [[https://bugs.launchpad.net/evergreen/+bug/2019139|Example ESLint rule]] (not yet committed)
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/d04b1469d84c0adf9b161ea16f0e2d6b5fd71fb8|Example nightwatch test]] that accompanies a fix of two accessibility issues: an image missing alt text and missing landmark roles.
 ===== Angular typescript =====
+Angular unit tests are useful for guarding against regressions in the typescript code, whether it is in a component, service, or elsewhere.
+Note that Angular unit tests are relatively easy to write for components, services, etc. that have a single responsibility.  Unfortunately, the Angular client has many very complex typescript classes with many responsibilities and many dependencies, which makes it much more difficult to test them in isolation.  Simple mocks of common services can be found in Open-ILS/src/eg2/src/test_data/mock_generators.ts.
+As with templates, if your bug was caused by a general bad practice that could happen elsewhere in the Angular typescript code, you can write an ESLint rule.
+Another option can be to use the type system to prevent future regressions.  For example, if your bug happened because a method received an object that was missing some important data, you can change the type signature of the method to ensure that the method's callers pass data in the correct format.
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/73c5fdd1c7882182031a74a3ea1dc3cc884acadb|Example typescript bugfix that includes an angular test]]
 ===== Database functions =====
@@ Line 19: / Line 37: @@
 ===== Perl logic =====
+If you fix a bug in the Perl logic, you will want to write a Perl test to accompany it.  If you can test the logic in isolation easily, put your test into Open-ILS/src/perlmods/t.  If you need to make connections to OpenSRF, the Database, memcached, etc., your test should instead go into Open-ILS/src/perlmods/live_t.  Live tests are quite a bit slower, so it is nice to do a traditional unit test when possible.  However, it is often not possible, unless the perl modules in question have great Separation of Concerns.
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/a810c0bae0682d25d600d448903b20ef447bd0cf|Example of a simple test (in the t directory)]] that accompanies a bugfix.  This test is perhaps not ideal because it tests a private method.
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/d438e83692c58e04b50010451b88b121547f6c2c|Example of a live test]] that accompanies a bugfix.
 ===== C logic =====
@@ Line 24: / Line 46: @@
   * [[https://github.com/evergreen-library-system/Evergreen/commit/0458ae01ef6a84734efb7232bc1cdaf479dd3be8|Example of a Perl live test testing a bugfix in the C code]]
-===== IDL problems =====
+===== IDL and other XML problems =====
+To ensure that the IDL (or other XML file) is in a consistent format, you can adjust the XML schema (*.xsd) files.  For example, [[https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=e71d0ce1360c4a28d4b23c440e227548228c70fc|this commit adjusts the schema]] to specify that classes in the IDL may have an attribute named "cardinality", but that it must have one of three values: low, high, or unbounded.
+To guard against more complex regressions in the IDL, you could write a perl unit test that parses the fm_IDL.xml file and makes assertions against it.  I don't know of any examples of this type of test (yet!) at the time of writing.
 ===== TT2 problems =====
+TT2 files can be tested using a Perl unit test.
+Here's an example of a basic test (intended as a starting point, it has a few issues as noted below):
+<code>
+use strict; use warnings;
+use Test::More tests => 1;
+use Template;
+use FindBin qw($Bin);
+use Carp;
+my $filename = 'opac/parts/myopac/prefs_hints.tt2';
+my $tt = Template->new({
+    INCLUDE_PATH => ["$Bin/../../templates-bootstrap", "$Bin/../../templates"],
+    PLUGINS => {
+        CGI_utf8 => 'OpenILS::WWW::EGWeb::CGI_utf8'
+    }
+});
+my $output;
+$tt->process(
+    $filename,
+    {}, # This is where you would set any variables that are needed for the template
+    \$output
+) or croak $tt->error;
+like $output, qr/<p/, 'it is structured as a <p> tag';
+</code>
+Be careful of testing very specific details of the templates.  If your assertion could theoretically fail without necessarily indicating a problem, find a different way of testing it.  For example, the above example could fail in several ways that don't necessarily indicate a problem:
+  * The filename of the tt2 file changes
+  * The tt2 file switches to some other markup that is just as good as a ''<p>'' tag
+The above example test could also be tricked!  For example, if the template had a ''<pre>'' tag but no ''<p>'' tag, the test would pass despite having incorrect markup.  Similarly, this test never asserts that the ''<p>'' tag is closed within the template, or indeed that the HTML syntax is correct in any way.  A more reliable test could perhaps use ''HTML::TreeBuilder'' to parse the ''$output'' string and make more trustworthy (and readable) assertions.
+Note that you can iterate through multiple tt2 templates (or even all of them!) in the same test, so it can be a good option if there are certain bad practices that you want to catch regardless throughout the tt2 code base.
+  * [[https://github.com/evergreen-library-system/Evergreen/blob/main/Open-ILS/src/perlmods/t/22-template-toolkit-parse.t|An example test that checks every tt2 file in the src directory]]
 ===== OPAC Javascript problems =====
+Some javascript files in the OPAC are EcmaScript modules (i.e. they use ''import'' and ''export'').  You can validate fixes to these files by adding a test to Open-ILS/web/opac/tests.
+If the javascript file in question is not yet an EcmaScript module, you may consider refactoring it to be one -- not only does this help with testability, but EcmaScript modules have the added benefit of not blocking the browser from rendering.
+  * [[https://github.com/evergreen-library-system/Evergreen/commit/dcca92e1d995a1343023c442661c73d024caadf9|Example of an OPAC javascript bugfix with a unit test]]
 ===== General =====
   * Adding a comment about why you use a particular approach is always welcome.
   * A clear, descriptive commit message without unnecessary jargon.
+  * If your test required some extra data beyond what is available in the Concerto Data Set or Enhanced Concerto Data Set, consider adding that data to make things easier for human testers.