Page MenuHomeSoftware Heritage

cypress: Test revision diff
ClosedPublic

Authored by kalpitk on Jul 31 2019, 11:43 AM.

Details

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kalpitk created this revision.Jul 31 2019, 11:43 AM
kalpitk updated this revision to Diff 6053.Jul 31 2019, 7:18 PM
  • cypress: Test origin-diff
  • add tests
kalpitk added inline comments.Jul 31 2019, 7:20 PM
cypress/integration/origin-diff.spec.js
47 ↗(On Diff #6053)

This test crashes the server with segmentation fault sometimes.
Can't figure out how a e2e test can lead to that.

kalpitk added inline comments.Jul 31 2019, 7:26 PM
cypress/integration/origin-diff.spec.js
47 ↗(On Diff #6053)

(happens most of the times , when this spec file is run alone)

anlambert requested changes to this revision.Aug 1 2019, 4:08 PM
anlambert added a subscriber: anlambert.

Thanks for working on this!

Sorry for the delay about the segfault issue, it took me some time to understand what the real problem was and I had to implement
a proper fix while maintaining backward compatibility with our production environment.

I added some inline comments to improve the implementation of some tests.

Also can you rename the test file to revision-diff.spec.js.

cypress/integration/origin-diff.spec.js
30 ↗(On Diff #6053)

You can chain the calls to the cypress API here

36–37 ↗(On Diff #6053)

Here I would put all paths in a Set first to avoid running some tests twice (from_path and to_path are equal when a file is modified)

47 ↗(On Diff #6053)

I managed to reproduce the issue from my side but it took me some time to analyze it and fix it properly.

The segfault was due to the use of the file-magic package in swh-web.
We use it to detect the mime_type and the encoding of contents stored into the archive.
I already experienced issues with it by the past and had to add an ugly hack to workaround them (see a2759fec6009).
At the time, I did not understand that calls to the file-magic API are not thread-safe and the issue you encountered
helped me to find the real cause of the issue.

I have just committed 0721db0ce7e8 and cd747b23ec7f properly fixing the issue. While maintaining backward compatibility
for the file-magic use (this is what we use in production on debian stretch environment) and making it thread-safe, I decided to favorize the use of the python-magic package which is thread-safe and more popular among Python developers (plus it is the one packaged on debian buster).

48–59 ↗(On Diff #6053)

Here I would rather use the implementation below as it is easier to understand:

it('should load diffs when scrolled down', function() {
    cy.wait(500)
      .get('#swh-revision-changes-list a')
      .each(elt => {
        cy.get(elt.attr('href'))
          .scrollIntoView()
          .wait(500)
          .find('.swh-content')
          .should('be.visible');
      });
  });
This revision now requires changes to proceed.Aug 1 2019, 4:08 PM
kalpitk updated this revision to Diff 6078.Aug 2 2019, 12:34 PM
  • Add tests
  • adapt to suggested changes
anlambert retitled this revision from cypress: Test origin-diff to cypress: Test revision diff.Aug 2 2019, 2:59 PM
anlambert accepted this revision.Aug 2 2019, 4:38 PM

Nice work, thanks !

You will have to rebase before landing this as I have added the generation of HTML reports after the cypress tests have been executed:

  • test results nicely displayed by mochawesome (6694c17fc939 )
  • javascript code coverage generated by istanbul (a01295397a91)

Those reports will be made available to browse on Jenkins cypress jobs web interface (see D1804)

This revision is now accepted and ready to land.Aug 2 2019, 4:38 PM
kalpitk updated this revision to Diff 6087.Aug 2 2019, 6:10 PM
  • rebase with master
This revision was automatically updated to reflect the committed changes.