Bug 40046 - Remove wait and screenshot from Tools/ManageMarcImport_spec.ts
Summary: Remove wait and screenshot from Tools/ManageMarcImport_spec.ts
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal
Assignee: Jonathan Druart
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on: 39606
Blocks: 40051
  Show dependency treegraph
 
Reported: 2025-06-02 10:14 UTC by Jonathan Druart
Modified: 2025-06-12 15:00 UTC (History)
1 user (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:


Attachments
Bug 40046: (bug 39606 follow-up) Remove waits and screenshots (3.60 KB, patch)
2025-06-02 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40046: (bug 39606 follow-up) Remove waits and screenshots (3.65 KB, patch)
2025-06-02 19:01 UTC, David Nind
Details | Diff | Splinter Review
Bug 40046: Prevent random failures when verifying select's values (1.96 KB, patch)
2025-06-03 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 40046: Prevent random failures when verifying select's values (2.91 KB, patch)
2025-06-04 08:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
UI failure (286.56 KB, image/png)
2025-06-12 10:19 UTC, Pedro Amorim
Details
Bug 40046: Use xml and auto-select it (3.09 KB, patch)
2025-06-12 11:37 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2025-06-02 10:14:15 UTC

    
Comment 1 Jonathan Druart 2025-06-02 10:16:52 UTC
Created attachment 182903 [details] [review]
Bug 40046: (bug 39606 follow-up) Remove waits and screenshots

From bug 39606:
"""
Why are you taking screenshots?
The wait statements will certainly cause random failures.

cy.wait(5000);
cy.wait(2000);

Notice the following couple of lines:
  cy.get("#fileuploadstatus").contains("100%");
  cy.get("legend").contains("Look for existing records in catalog?").should('be.visible');

It makes Cypress wait 10s until "100%" appears (ie. the file is updated), then the block with the dropdown lists you are going to interact with to be visible.

You should almost never use cy.wait.
"""

Test plan:
  yarn cypress run --spec t/cypress/integration/Tools/ManageMarcImport_spec.ts
should still pass
Comment 2 David Nind 2025-06-02 19:01:07 UTC
Created attachment 182912 [details] [review]
Bug 40046: (bug 39606 follow-up) Remove waits and screenshots

From bug 39606:
"""
Why are you taking screenshots?
The wait statements will certainly cause random failures.

cy.wait(5000);
cy.wait(2000);

Notice the following couple of lines:
  cy.get("#fileuploadstatus").contains("100%");
  cy.get("legend").contains("Look for existing records in catalog?").should('be.visible');

It makes Cypress wait 10s until "100%" appears (ie. the file is updated), then the block with the dropdown lists you are going to interact with to be visible.

You should almost never use cy.wait.
"""

Test plan:
  yarn cypress run --spec t/cypress/integration/Tools/ManageMarcImport_spec.ts
should still pass

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Pedro Amorim 2025-06-03 12:09:30 UTC
This is causing tests to fail for me. Consistently.
Using UI:

assertexpected <select#matcher> to have value '3', but the value was ''
AssertionError
Timed out retrying after 10000ms: expected '<select#matcher>' to have value '3', but the value was ''

Using yarn cypress run --spec t/cypress/integration/Tools/ManageMarcImport_spec.ts:

  1) loads the manage MARC import page
       upload a MARC record:
     AssertionError: Timed out retrying after 10000ms: Expected to find content: 'View batch' but never did.
      at Context.eval (webpack://koha/./t/cypress/integration/Tools/ManageMarcImport_spec.ts:76:11)


Both UI and CLI pass if I revert the patch.
Comment 4 Jonathan Druart 2025-06-03 13:54:34 UTC
(In reply to Pedro Amorim from comment #3)
> This is causing tests to fail for me. Consistently.
> Using UI:
> 
> assertexpected <select#matcher> to have value '3', but the value was ''
> AssertionError
> Timed out retrying after 10000ms: expected '<select#matcher>' to have value
> '3', but the value was ''
> 
> Using yarn cypress run --spec
> t/cypress/integration/Tools/ManageMarcImport_spec.ts:
> 
>   1) loads the manage MARC import page
>        upload a MARC record:
>      AssertionError: Timed out retrying after 10000ms: Expected to find
> content: 'View batch' but never did.
>       at Context.eval
> (webpack://koha/./t/cypress/integration/Tools/ManageMarcImport_spec.ts:76:11)
> 
> 
> Both UI and CLI pass if I revert the patch.

I think it is not related to this patch.

It's not consistent for me.
After a bit of debug I see that we hit
C4/ImportBatch.pm
 374         if ( scalar( $marc_record->fields() ) == 0 ) {                                               
 375             push @invalid_records, $marc_record; 
 376         } else { 

$marc_record is
        _fields     [],                                                                                                                                                                                              
        _leader     "         a              ",                                                                                                                                                                      
        _warnings   [                                                                                                                                                                                                
            [0] "Record length \"<?xml\" is not numeric in record 1"                                                                                                                                                 
        ]
Comment 5 Jonathan Druart 2025-06-03 14:14:59 UTC
Created attachment 182921 [details] [review]
Bug 40046: Prevent random failures when verifying select's values

Commands like .select() are queued. We need to prevent the should to
happen before the select is executed.
Comment 6 Jonathan Druart 2025-06-03 14:15:14 UTC
(In reply to Jonathan Druart from comment #5)
> Created attachment 182921 [details] [review] [review]
> Bug 40046: Prevent random failures when verifying select's values
> 
> Commands like .select() are queued. We need to prevent the should to
> happen before the select is executed.

This patch fixes another failure:

      Timed out retrying after 10000ms               
      + expected - actual                                                                                 
                                                                                                          
      +'3'                                                                                                
                                                                                                          
      at Context.eval (webpack://koha/./t/cypress/integration/Tools/ManageMarcImport_spec.ts:67:27)
Comment 7 Jonathan Druart 2025-06-04 08:47:33 UTC
Created attachment 182932 [details] [review]
Bug 40046: Prevent random failures when verifying select's values

Commands like .select() are queued. We need to prevent the should to
happen before the select is executed.
Comment 8 Jonathan Druart 2025-06-04 08:50:34 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Pedro Amorim from comment #3)
> > This is causing tests to fail for me. Consistently.
> > Using UI:
> > 
> > assertexpected <select#matcher> to have value '3', but the value was ''
> > AssertionError
> > Timed out retrying after 10000ms: expected '<select#matcher>' to have value
> > '3', but the value was ''
> > 
> > Using yarn cypress run --spec
> > t/cypress/integration/Tools/ManageMarcImport_spec.ts:
> > 
> >   1) loads the manage MARC import page
> >        upload a MARC record:
> >      AssertionError: Timed out retrying after 10000ms: Expected to find
> > content: 'View batch' but never did.
> >       at Context.eval
> > (webpack://koha/./t/cypress/integration/Tools/ManageMarcImport_spec.ts:76:11)
> > 
> > 
> > Both UI and CLI pass if I revert the patch.
> 
> I think it is not related to this patch.
> 
> It's not consistent for me.
> After a bit of debug I see that we hit
> C4/ImportBatch.pm
>  374         if ( scalar( $marc_record->fields() ) == 0 ) {                 
> 
>  375             push @invalid_records, $marc_record; 
>  376         } else { 
> 
> $marc_record is
>         _fields     [],                                                     
> 
>         _leader     "         a              ",                             
> 
>         _warnings   [                                                       
> 
>             [0] "Record length \"<?xml\" is not numeric in record 1"        
> 
>         ]

This actually happened when the form was submitted with format=MARC, before MARCXML was actually selected.

I had both CLI and UI failing randomly.

With this last patch applied they pass consistently.

Can you confirm that, Pedro?
Comment 9 Pedro Amorim 2025-06-12 10:19:50 UTC
Created attachment 183195 [details]
UI failure

Still fails for me, sorry (same as before). Attached is screenshot of UI failure. CLI failure is the same as before.
Comment 10 Jonathan Druart 2025-06-12 11:37:18 UTC
Created attachment 183196 [details] [review]
Bug 40046: Use xml and auto-select it

Absolutely no idea why the previous code was not selecting MARCXML (MARC
was passed and so the import failed)

        cy.get('select[name="format"]')
            .select("MARCXML", { force: true })
            .should("have.value", "MARCXML");

But I found a bug in the JS code: we are supposed to auto-detect .xml
file and pre-select MARCXML. So let's rely on that behaviour!

sample.mrc is actually an xml file so it makes sense to rename it
Comment 11 Pedro Amorim 2025-06-12 14:55:08 UTC
Been trying this out and chatting with Joubu on Mattermost. It still fails for me often (not always, like 50%) with the same error 'View batch' as before. From the screenshots and videos it appears the background job fails to start for some reason, possibly low resources on my machine at the time of running the tests (?). The background job bar shows '0% failed' in red.

Dumping $marc_record in the code mentioned by comment 4 produces the following output, when the tests fail:

bless( {
                 '_leader' => '         a              ',
                 '_fields' => [],
                 '_warnings' => [
                                  'Record length "<?xml" is not numeric in record 1'
                                ]
               }, 'MARC::Record' );

I don't feel comfortable PQAing this for the above reasons but I'm not against it if we decide to push and address this later.
Comment 12 Jonathan Druart 2025-06-12 15:00:12 UTC
(In reply to Pedro Amorim from comment #11)
> Been trying this out and chatting with Joubu on Mattermost. It still fails
> for me often (not always, like 50%) with the same error 'View batch' as
> before. From the screenshots and videos it appears the background job fails
> to start for some reason, possibly low resources on my machine at the time
> of running the tests (?). The background job bar shows '0% failed' in red.
> 
> Dumping $marc_record in the code mentioned by comment 4 produces the
> following output, when the tests fail:
> 
> bless( {
>                  '_leader' => '         a              ',
>                  '_fields' => [],
>                  '_warnings' => [
>                                   'Record length "<?xml" is not numeric in
> record 1'
>                                 ]
>                }, 'MARC::Record' );
> 
> I don't feel comfortable PQAing this for the above reasons but I'm not
> against it if we decide to push and address this later.

What's happening is the form being submitted with "MARC" instead of "MARCXML", even with the last change, which does not make sense at all.

So we may have some weird JS code something that reverts to MARC, but I haven't found any.

I am suggesting to push as it, to move forward and also unlock the follow-up bug, then add a test at the QA script level to prohibit those cy.wait(\d) that are totally wrong and must (almost) never be used. At least to prevent new occurrences.

Then we may want to reintroduce this one back (or simply remove/adjust the tests) if it fails on jenkins.