Skip to content

Conversation

@adamretter
Copy link
Contributor

No description provided.

@adamretter adamretter requested a review from joewiz April 8, 2021 14:13
@adamretter adamretter force-pushed the ci branch 3 times, most recently from 44069be to acb494b Compare April 8, 2021 14:54
@joewiz joewiz merged commit 667567a into eXist-db:master Apr 8, 2021
Copy link
Contributor

@duncdrum duncdrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think these changes have the effect you are hoping for, also there is a lot more going on here then just switchin CI providers.

All the changes were made so that we can have builds that work both locally and in CI, this was needed to let us change CI provider in a sensible way.

Please consider that I am trying to improve things, there is a great deal that was broken before I started. Before my PR:

  1. CI isn't working - e.g. Travis is no longer
  2. Running mvn compile corrupts the src/ folders by using Gulp to apply liniting and minimisation to the javascript files. Such changes should only be applied to target/ otherwise it messes with your Git unstanged changes and can cause you to commit things that you shouldn't. NOTE: I have not attempted to fix that yet, as I thought it required discussion on a call first.
  3. There are duplicate copies of a lot of Javascript and CSS files in the codebase.
  4. Running mvn package or even mvn test on a clean machine would always fail, rendering it impossible to build a docs XAR package.

NOTE: I have not attempted to fix (2) or (3) yet, as I thought those were best discussed on a CC call first.

  1. I don't see how the working code changes are actually installed into the container. This means you are testing the version of the docs that ships with exist by default, not the code you are building.

You are right! I missed a commit during my rebase :-/ I will send it now...

  1. We now have a runtime dependency on docker, which is a) undocumented and b) unnecessary. You don't need to have docker installed and running to edit the docs, now you do

You still don't need Docker installed to edit and validate the docs and then build a package.

mvn package

Now validates the docs for DocBook 5 etc, and builds the package. No Docker required.

  1. Why was the cypress projectId changed? this means we break with past recordings, the project hasn't changed, the ID should remain, was there a problem with the old ID?

I went to Cypress.io and there was no project for existdb, so I had to create one. I added you to it as an admin, you should have got an email.

  1. I don't think that XQSuite tests are integration tests, they should stay in the test phase where unit-tests are run. This would be consistent with any other mavenized repo that uses xqsuite tests including exist-core.

They are absolutely integration tests by their very nature because they have an external dependency from this project on a running service (i.e. eXist-db). The way exist-core is structured is not perfect due to the fact that it came from Ant where it was just one big lump of code, further major refactoring is still needed to get exist-core up to modern reusable software standards; so I wouldn't use that as a comparison!

Setting these as integration tests, also binds them to a different lifecycle phase, meaning that it is optional to run them and doesn't require either a running eXist-db installation (or Docker) for everyone.

  1. why do we have two steps on ci: clean package and verify. Just verify should be enough as per the PR description (and safe us wall clock time)

We need to split tests (e.g. validate Docbook) from integration test (e.g. I need a running eXist-db or Docker installed). The former is perfect for people who want to contribute to the docs, and the latter for developers and CI.

  1. The scaffold for js unit tests serves a purpose, it shows js contributors what is installed already and how we want tests, what do we gain by removing it, see 7.

Sorry, what was removed? The tasks that can be performed and the behaviour should be the same

  1. The tests in the documentation repo, as well as the Ci config, are referenced in the documentation articles about testing, these references need to be updated (this also contains the information why we did a separate call to validate on travis, despite the fact that this was not strictly necessary. To make it easy for new contributors. The current setup produces 4800 log lines about downloading the internet, and about 20 log lines about cypress tests, you no longer see that validation of the docbook files against different schemas, nor do you see which unit-test are actually run. This does not strike me as in line with lowering the barrier of entry

Okay I can update the docs. Keep in mind before this PR the build did not build or test. Now you can just:

  1. mvn package - to test DocBook and schematron compliance and create a XAR
  2. mvn verify - does the above and then also runs the integration tests against eXist-db (requires a running eXist-db install or defaults to doing via Docker for you).

This is how it should be, there is a clear build execution lifecycle and the right things are done at the right phases; ignoring the linting and minimisation - which is done at the wrong phase with the wrong outputdir at the moment - but that was the case before this PR anyway.

  1. is a real showstopper imv @adamretter @joewiz

Well it's better than it was - i.e. it wasn't running ;-) But yes, the missing commit is on the way...

adamretter added a commit to adamretter/documentation that referenced this pull request Apr 9, 2021
adamretter added a commit to adamretter/documentation that referenced this pull request Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants