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

          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