Moodle
  1. Moodle
  2. MDL-40033

Behat tests fail randomly for upload/filepicker/filemanager-related feature

    Details

    • Rank:
      50789

      Description

      Feature:
      In an assignment, students can upload files for assessment

      Scenario:
      Submit a file and update the submission with another file

      Failure:
      And
      ".ffilemanager .fm-maxfiles .fp-btn-add" "css_element" should exists
      behat_general::should_exists()

      Css matching locator ".ffilemanager .fm-maxfiles .fp-btn-add" not found.

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          my first reaction was that element with path ".ffilemanager .fm-maxfiles .fp-btn-add" is always hidden by CSS (the rule is .filemanager.fm-maxfiles .fp-btn-add

          {display:none;}

          )

          But it is hidden always which does not explain why the test fails randomly.

          I was able to reproduce this error by removing all delays between uploading a file and looking for css element (i.e. in behat_repository_upload::i_upload_file_to_filepicker() and behat_base::find_all() ). But it means that in testing environment file upload is taking over 12s which is also unreal. Or file upload fails for other reason

          So I can suggest a patch but have no idea how to test it because I can't reproduce it

          Show
          Marina Glancy added a comment - my first reaction was that element with path ".ffilemanager .fm-maxfiles .fp-btn-add" is always hidden by CSS (the rule is .filemanager.fm-maxfiles .fp-btn-add {display:none;} ) But it is hidden always which does not explain why the test fails randomly. I was able to reproduce this error by removing all delays between uploading a file and looking for css element (i.e. in behat_repository_upload::i_upload_file_to_filepicker() and behat_base::find_all() ). But it means that in testing environment file upload is taking over 12s which is also unreal. Or file upload fails for other reason So I can suggest a patch but have no idea how to test it because I can't reproduce it
          Hide
          David Monllaó added a comment -

          Thanks, I'm seeing most of the failures in this scenario in the "I upload "lib/tests/fixtures/upload_users.csv" file to "File submissions" filepicker step and a few of them also in ".ffilemanager .fm-maxfiles .fp-btn-add" "css_element" should exists so I'm guessing that the problem is before that and I'm looking for something wrong in the steps definitions that can be related with the submission edition, still looking at it...

          Show
          David Monllaó added a comment - Thanks, I'm seeing most of the failures in this scenario in the "I upload "lib/tests/fixtures/upload_users.csv" file to "File submissions" filepicker step and a few of them also in ".ffilemanager .fm-maxfiles .fp-btn-add" "css_element" should exists so I'm guessing that the problem is before that and I'm looking for something wrong in the steps definitions that can be related with the submission edition, still looking at it...
          Hide
          Marina Glancy added a comment -

          David, I don't agree with you replacing ".ffilemanager .fm-maxfiles .fp-btn-add" with ".ffilemanager .fp-btn-add"
          The class .fm-maxfiles is added when number of files in filemanager has reached the maximum limit. I thought test was actually intended to check that.

          Show
          Marina Glancy added a comment - David, I don't agree with you replacing ".ffilemanager .fm-maxfiles .fp-btn-add" with ".ffilemanager .fp-btn-add" The class .fm-maxfiles is added when number of files in filemanager has reached the maximum limit. I thought test was actually intended to check that.
          Hide
          David Monllaó added a comment -

          Grrr, yes I agree with you, I thought the add button was removed from the DOM (not hidden) and I thought that checking only that was enough. Thanks Marina, I'll update the patch

          Show
          David Monllaó added a comment - Grrr, yes I agree with you, I thought the add button was removed from the DOM (not hidden) and I thought that checking only that was enough. Thanks Marina, I'll update the patch
          Hide
          David Monllaó added a comment -

          Requesting peer review and changing priority to Major

          Show
          David Monllaó added a comment - Requesting peer review and changing priority to Major
          Hide
          David Monllaó added a comment -

          Sending straight to integration as has been tested in Damyon's machine and is important to return to 100% there, also it solves problems with Windows environments as they don't support unix-style line breaks.

          Show
          David Monllaó added a comment - Sending straight to integration as has been tested in Damyon's machine and is important to return to 100% there, also it solves problems with Windows environments as they don't support unix-style line breaks.
          Hide
          Damyon Wiese added a comment -

          Belated peer review.

          All looks OK to me except that there are cleanups in this patch that seem unrelated to the issue and would be better split out. (The multi line string cleanups).

          Show
          Damyon Wiese added a comment - Belated peer review. All looks OK to me except that there are cleanups in this patch that seem unrelated to the issue and would be better split out. (The multi line string cleanups).
          Hide
          David Monllaó added a comment -

          Thanks Damyon, you can reset the status to development in progress?

          Show
          David Monllaó added a comment - Thanks Damyon, you can reset the status to development in progress?
          Hide
          David Monllaó added a comment -

          Reopening according to Damyon's review

          Show
          David Monllaó added a comment - Reopening according to Damyon's review
          Hide
          David Monllaó added a comment -

          Back to integration, the multiline cleanup will be part of MDL-39635

          Show
          David Monllaó added a comment - Back to integration, the multiline cleanup will be part of MDL-39635
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          David Monllaó added a comment -

          rebased

          Show
          David Monllaó added a comment - rebased
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (testing instructions required)

          Show
          Eloy Lafuente (stronk7) added a comment - (testing instructions required)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Oki, assuming that running behat tests with:

          --tags '@javascript,@repository,@core_filepicker'

          is enough to verify all affected steps are ok.

          So going ahead with this. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Oki, assuming that running behat tests with: --tags '@javascript,@repository,@core_filepicker' is enough to verify all affected steps are ok. So going ahead with this. Ciao
          Hide
          David Monllaó added a comment -

          Sorry Eloy, added testing instructions.

          About this issue I forgot to comment about the wait() decisions, related with MDL-40246, in this case I haven't managed to archive the expected behaviour without the hardcoded waits, it seems that before the file upload form is loaded and after the file manager contents are updated there is an extra DOM nodes update that changes the DOM (also the DOM node IDs) and Selenium can not interact ($element->click()) with the same element that previously interacted ($element = $this->find...) to get the DOM node id.... specially strange.

          Show
          David Monllaó added a comment - Sorry Eloy, added testing instructions. About this issue I forgot to comment about the wait() decisions, related with MDL-40246 , in this case I haven't managed to archive the expected behaviour without the hardcoded waits, it seems that before the file upload form is loaded and after the file manager contents are updated there is an extra DOM nodes update that changes the DOM (also the DOM node IDs) and Selenium can not interact ($element->click()) with the same element that previously interacted ($element = $this->find...) to get the DOM node id.... specially strange.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated (25 & master), thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated (25 & master), thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And passing. I've run --tags '@_only_local,@repository,@core_filepicker' 3 times on each branch and all them have ended ok (0 failures).

          Show
          Eloy Lafuente (stronk7) added a comment - And passing. I've run --tags '@_only_local,@repository,@core_filepicker' 3 times on each branch and all them have ended ok (0 failures).
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks for giving me joys and smiles
          Thanks for sharing my trouble's pile

          Thanks for wipeing the tears of my eye
          Thanks for showing me the glad view of sky

          Thanks for lending me your shoulders to lean
          Thanks for giving my words a proper mean

          Thanks for telling me the value of life
          Thanks for showing me the rules to survive

          Thanks for lending me the sympathetic ears
          Thanks for showing how much you care

          From all this what I mean in the end
          Is thanks for being my special friend.

          – Seema Chowdhury

          Sent upstream so... closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks for giving me joys and smiles Thanks for sharing my trouble's pile Thanks for wipeing the tears of my eye Thanks for showing me the glad view of sky Thanks for lending me your shoulders to lean Thanks for giving my words a proper mean Thanks for telling me the value of life Thanks for showing me the rules to survive Thanks for lending me the sympathetic ears Thanks for showing how much you care From all this what I mean in the end Is thanks for being my special friend. – Seema Chowdhury Sent upstream so... closing, thanks!

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: