CI/CD Fix unit test workflow

[Splines]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Old:
Improve the unit test GitHub Actions workflow and get it to run without failures. To do so, I temporarily disabled failing unit tests. These should be fixed according to issue #442

Edit:
This PR will fix the unit test workflow. Other than originally stated, it will not disable any unit tests.

[Splines]

Hi @henrixapp, what is the "reindex sunspot" task used for, which you've introduced here? Why do we need it?

docker-compose exec mampf  sh -c "RAILS_ENV=test  rake sunspot:reindex" 

Furthermore, I've just noticed that I replaced docker compose run with docker compose exec.

docker-compose exec will run inside your existing, running service container while docker-compose run will start a new, independent container.
from here

Based on this, exec sounded better to me and it seems to work (I use docker compose up --build --detach now in a previous step and then use docker compose exec to execute the database related commands in the new mampf container). However, you explicitly used run instead of exec starting from here. Is there a reason for that? (maybe even indicating I should refrain from exec in this case?)

[Splines]

I think it should be ok to merge this directly to main instead of mampf-next, so that other branches can directly profit from this. This does not touch on any code and instead just affects the CI/CD pipeline (which is currently not working correctly anyways ;))

[henrixapp]

Hi @Splines,

what is the "reindex sunspot" task used for, which you've introduced here? Why do we need it?

Sunspot is used for indexing the data and providing search results. The reindexing has to be triggered to index the test data, ingested in the step before.

Based on this, exec sounded better to me and it seems to work (I use docker compose up --build --detach now in a previous step and then use docker compose exec to execute the database related commands in the new mampf container). However, you explicitly used run instead of exec starting from here. Is there a reason for that? (maybe even indicating I should refrain from exec in this case?)

I can not remember this directly. As the operations are altering the volumes/data of the database, I found it more convenient
to spin them up via run, but your way of doing it probably works fine.

I think it should be ok to merge this directly to main instead of mampf-next, so that other branches can directly profit from this. This does not touch on any code and instead just affects the CI/CD pipeline (which is currently not working correctly anyways ;))

I would suggest keeping the direction of merging clean, because otherwise you will have to sync between main and mampf-next at a later point. Furthermore, as you can see by the pull request #205, my changes between run and exec were in one pull request. It might be worth considering squashing commits, keeping the history cleaner and more understandable. Especially, #311 is a candidate for squashing.

Finally, let's take a look on what you try to achieve: You want the tests to be passing, to get that ✅ .

You commented out the tests that are failing, achieving your goal, but I would suggest going a different route, fixing the setup/environment error.

If we take a look at the logs of a failing job, we see that the unit test fails due to a webpacker error:

     # ------------------
1428
     # --- Caused by: ---
1429
     # Webpacker::Manifest::MissingEntryError:
1430
     #   Webpacker can't find application.js in /usr/src/app/public/packs-test/manifest.json. Possible causes:
1431
     #   1. You want to set webpacker.yml value of compile to true for your environment
1432
     #      unless you are using the `webpack -w` or the webpack-dev-server.
1433
     #   2. webpack has not yet re-run to reflect updates.
1434
     #   3. You have misconfigured Webpacker's config/webpacker.yml file.
1435
     #   4. Your webpack configuration is not creating a manifest.
1436
     #   Your manifest contains:
1437
     #   {
1438
     #   }
1439
     #   /usr/local/bundle/gems/webpacker-5.4.3/lib/webpacker/manifest.rb:79:in `handle_missing_entry'

So apparently we do not build the manifest file.
This can be fixed by adding a precompile directive to the steps while building.

docker compose run --entrypoint="" mampf  sh -c "RAILS_ENV=test  rake assets:precompile"

I use https://github.com/nektos/act to run actions locally and the tests passed on main with your updated file.

[Splines]

@henrixapp

I would suggest keeping the direction of merging clean, because otherwise you will have to sync between main and mampf-next at a later point.

True, fair enough ;)

It might be worth considering squashing commits, keeping the history cleaner and more understandable.

Yes, especially for the "release" branch mampf-next, we could squash commits from the individual feature branches. Then, merge mampf-next to the main branch with a merge commit.

I would suggest going a different route, fixing the setup/environment error.

Thanks for also giving me the fix, will try this out and commit. I overlooked this and thought that some tests were just not correct.

I use https://github.com/nektos/act to run actions locally

Wow, didn't know about that project, thank you, will try it out. This should render ugly force pushes (just to trigger the pipeline) unnecessary.

[codecov]

Codecov Report

Merging #441 (5e158c5) into mampf-next (7db4b45) will increase coverage by 1.24%.
The diff coverage is 72.75%.

❗ Current head 5e158c5 differs from pull request most recent head f395931. Consider uploading reports for the commit f395931 to get more accurate results

@@              Coverage Diff               @@
##           mampf-next     #441      +/-   ##
==============================================
+ Coverage       65.44%   66.68%   +1.24%     
==============================================
  Files             302      311       +9     
  Lines            8965     9350     +385     
==============================================
+ Hits             5867     6235     +368     
- Misses           3098     3115      +17     

Impacted Files	Coverage Δ
app/helpers/lectures_helper.rb	35.18% <ø> (+0.18%)	⬆️
app/helpers/media_helper.rb	32.87% <0.00%> (+0.93%)	⬆️
app/models/manuscript.rb	19.81% <0.00%> (-0.28%)	⬇️
app/models/term.rb	68.57% <0.00%> (-7.15%)	⬇️
spec/controllers/media_controller_spec.rb	100.00% <ø> (ø)
app/models/item.rb	52.29% <20.00%> (-0.27%)	⬇️
app/models/user_cleaner.rb	22.38% <22.38%> (ø)
app/controllers/profile_controller.rb	23.47% <25.00%> (+0.05%)	⬆️
app/helpers/users_helper.rb	42.85% <25.00%> (-23.81%)	⬇️
app/helpers/search_helper.rb	50.00% <33.33%> (-25.00%)	⬇️
... and 22 more

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[Splines]

@henrixapp Apparently, removing webpacker from the dev dependencies of the package.json does not work here but in your pull request #453. Do you know why? Maybe GitHub has cached Webpacker for you?

I've also added a yarn installation to the Dockerfile:

RUN bundle install &&\
    yarn install --production=false

[henrixapp]

Apparently, removing webpacker from the dev dependencies of the package.json does not work here but in your pull request #453. Do you know why? Maybe GitHub has cached Webpacker for you?

You are using the docker-compose.yml file/setup in docker/development and try to compile with the running container. The running container is mounting the whole mampf root folder here, overriding any node_modules folder with it.

Maybe the docker-compose setup in run_tests should be used.

[Splines]

Now, preparation for tests is working, however webpacker does not seem to be installed correctly (probably configuration file is not taken into account). Thus, the tests now fail, since the files are not bundled correctly...

Also see rails/webpacker#522 for more ideas ;)

[henrixapp]

@Splines the problem is that the Dockerfile in run_tests does not install the js with yarn dependencies. Forgot to mention that you need to update the Dockerfile properly.

[Splines]

This SO question might be relevant as it's discussing the same error we get currently: can't find application.js in /usr/src/app/public/packs-test/manifest.json

# --- Caused by: ---
# Webpacker::Manifest::MissingEntryError:
#   Webpacker can't find application.js in /usr/src/app/public/packs-test/manifest.json. Possible causes:
#   1. You want to set webpacker.yml value of compile to true for your environment
#      unless you are using the `webpack -w` or the webpack-dev-server.
#   2. webpack has not yet re-run to reflect updates.
#   3. You have misconfigured Webpacker's config/webpacker.yml file.
#   4. Your webpack configuration is not creating a manifest.
#   Your manifest contains:
#   {
#   }
#   /usr/local/bundle/gems/webpacker-5.4.4/lib/webpacker/manifest.rb:79:in `handle_missing_entry'

Edit: Wow, just noticed this is the exact same error we had in the beginning 😥

[Splines]

Maybe compiling test packs to a separate directory is a problem? See config/webpacker.yml:

test:
  <<<: *default

  # Production depends on precompilation of packs prior to booting for performance.
  compile: false

  # Extract and emit a css file
  extract_css: true

  # Cache manifest.json for performance
  cache_manifest: true

  # Compile test packs to a separate directory
  public_output_path: packs-test

[Splines]

This could also be related. Will try this in the next commit.

[Splines]

@henrixapp @christian-heusel Ok, I think I'm a bit stumped on this one (@christian-heusel I'm mentioning you as git blame tells me you worked on the Dockerfiles I'm currently working (despairing) on).

Somehow, webpacker is not properly working in the test environment and I can't figure out why, probably because I seem to miss the bigger picture of how exactly the whole pipeline works. This is still the error we get in the CI GitHub environment.

• In the development docker compose file, we have a separate webpacker service section which is probably starting webpacker and let it compile all frontend files. Why don't we have this section in the docker compose file for run_tests as well? Where is webpacker supposed to be started there?

• @henrixapp Here, you use COPY ... and RUN bundle install two times consecutively (lines 27/28 and 33-35), is this relevant to the current issue?

• @henrixapp I cannot find any changes you made in Cypress e2e tests #216 specifically related to Webpacker, except for maybe an additional yarn install in the Dockerfile, but that didn't work for me. Where is webpacker installed and where do you run the command to actually let webpacker go off compiling all the frontend .js and .scss files? What is the purpose of yarn install --production=false if we don't have any dev dependencies declared in the package.json?

I think a quick and dirty solution for this would be fine, as we will eventually replace webpacker anyways, see #454.

[henrixapp]

Closed in favor of #472