Bump upload artifact v3 to v4

[filipefurtad0]

What? Why?

• Closes Bump upload-artifact v3 to v4 #12701

We use actions/upload-artifact to create artifacts i.e., files like screenshots, when a spec fails. We've been getting a deprecation error. This PR fixes it.

Also, it removes the screenshot code from the engines node - I this is only needed on system specs, right?

What should we test?

• green build
• when a system spec fails, we get a screenshot of the screen, with the failure.

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[filipefurtad0]

I think it's working:

The build actually failed on the first run, and:

• the deprecation warning was gone:

• the screenshot file was uploaded:

[filipefurtad0]

I'm not being able to make this gem work correctly, we need to make sure all artifacts are created in separate files. Closing for now.

[chahmedejaz]

I'm not being able to make this gem work correctly, we need to make sure all artifacts are created in separate files. Closing for now.

I just did a small spike on this one, out of the box, the action doesn't support this feature.
We can use some custom code to make it work, however, the action would upload each file/screenshot in a separate zip file which we would have to download and extract to view the screenshot. This is because the action doesn't support non-zip file upload as well. Individual zip files would be more messy to deal with I guess. 😅

Right now all the failed test screenshots are uploaded in a single zip and we download and extract them all in one go. This seems more feasible as per the action's current behavior. Thanks

[filipefurtad0]

Right now all the failed test screenshots are uploaded in a single zip and we download and extract them all in one go.

Hey @chahmedejaz , thanks for looking into this. Yes I agree this is the ideal case. It seems I left this branch in a green state, so unfortunately I cannot demonstrate which the issue I encountered exactly.

I believe it had to do with artifacts created from failing specs, on different Knapsack nodes - i.e., the case in which the zip file needs to be updated in different moments, during the build run. This relates to this breaking change.

[chahmedejaz]

I believe it had to do with artifacts created from failing specs, on different Knapsack nodes - i.e., the case in which the zip file needs to be updated in different moments, during the build run. This relates to this breaking change.

Sure, I'll try to look into that tonight. However, I think in that case we should we should just change the name for the files being uploaded. I tried to validate this case and it worked fine. However, I don't remember now that whether it was v3 or v4 😅

Currently, it's the same as failed-tests-screenshots. It should be updated to failed-admin-tests-screenshots and failed-consumer-tests-screenshots respectively.

[filipefurtad0]

It should be updated to failed-admin-tests-screenshots and failed-consumer-tests-screenshots respectively.

Agree, renaming makes sense and would prevent the issue, but (I think!) only partially as we have 14 and 12 Knapsack nodes for admin and consumer specs, respectively (ci_node_total, on build.yml).

So, if different system (admin or consumer) specs fail on a single build run there is a good chance they land on different nodes. This is not the case if the failing tests are within the same file; in this case, failures will always be within the same node, as we don't set KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES: true (which would split files across nodes).

I think in that case we should we should just change the name for the files being uploaded

So if we can include the name of the failing spec, then I guess it should be fine 👍

This is my current understanding of it, I hope this is helpful 🙏

[chahmedejaz]

It should be updated to failed-admin-tests-screenshots and failed-consumer-tests-screenshots respectively.

Agree, renaming makes sense and would prevent the issue, but (I think!) only partially as we have 14 and 12 Knapsack nodes for admin and consumer specs, respectively (ci_node_total, on build.yml).

So, if different system (admin or consumer) specs fail on a single build run there is a good chance they land on different nodes. This is not the case if the failing tests are within the same file; in this case, failures will always be within the same node, as we don't set KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES: true (which would split files across nodes).

I think in that case we should we should just change the name for the files being uploaded

So if we can include the name of the failing spec, then I guess it should be fine 👍

This is my current understanding of it, I hope this is helpful 🙏

Hi @filipefurtad0 - I validated this and here are the results from this onwards. It was breaking as both jobs were trying to upload with the same name. With different names, it's fixed now. Thanks