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
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>
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.
(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" ]
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.
(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)
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.
(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?
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.
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
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.
(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.