Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-40033

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

    Details

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              marina 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 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
              dmonllao 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
              dmonllao 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 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 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
              dmonllao 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
              dmonllao 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
              dmonllao David Monllaó added a comment -

              Requesting peer review and changing priority to Major

              Show
              dmonllao David Monllaó added a comment - Requesting peer review and changing priority to Major
              Hide
              dmonllao 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
              dmonllao 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 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 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
              dmonllao David Monllaó added a comment -

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

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

              Reopening according to Damyon's review

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

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

              Show
              dmonllao David Monllaó added a comment - Back to integration, the multiline cleanup will be part of MDL-39635
              Hide
              poltawski 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
              poltawski 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
              dmonllao David Monllaó added a comment -

              rebased

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

              (testing instructions required)

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - (testing instructions required)
              Hide
              stronk7 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
              stronk7 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
              dmonllao 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
              dmonllao 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
              stronk7 Eloy Lafuente (stronk7) added a comment -

              Integrated (25 & master), thanks!

              Show
              stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (25 & master), thanks!
              Hide
              stronk7 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
              stronk7 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
              stronk7 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
              stronk7 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:
                    Fix Release Date:
                    8/Jul/13