Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor jasmine-test.sh #1491

Closed
wants to merge 2 commits into from
Closed

Refactor jasmine-test.sh #1491

wants to merge 2 commits into from

Conversation

strehle
Copy link
Member

@strehle strehle commented Jan 13, 2021

Refactor jasmine-test.sh after PR #1435

The test run showed, the the needed docker image is used, but
the ./scripts/jasmine-test.sh script is executed locally in the worker.
The worker has installed locally node 8.x with npm 5.x. The errors shows this.
The docker image cfidentity/uaa-singular has node 12.x with npm 6.4.

Finally: error occurs now, because new jasmine depends on newer node, e.g. node 10 or high.
Before the local test was working also with a node 8.x

Ensure that jasmine-test.sh check if it runs in docker and if not, start itself in cfidentity/uaa-singular

npm ci should be used instead npm install therefore update package-lock.json

The test run showed, the the needed docker image is used, but
the ./scripts/jasmine-test.sh script is executed locally in the worker.
The worker has installed locally node 8.x with npm 5.x. The errors shows this.
The docker image cfidentity/uaa-singular has node 12.x with npm 6.4.

Finally: error occurs now, because new jasmine depends on newer node, e.g. node 10 or high.
Before the local test was working also with a node 8.x

Ensure that jasmine-test.sh check if it runs in docker and if not, start itself in cfidentity/uaa-singular

npm ci should be used instead npm install therefore update package-lock.json
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/176477849

The labels on this github issue will be updated when the story is started.

@strehle strehle requested a review from thausler786 January 13, 2021 10:39
@strehle strehle changed the title Refactor jasmine-test.sh after PR #1435 Refactor jasmine-test.sh Jan 13, 2021
@strehle strehle removed the request for review from thausler786 January 13, 2021 10:44
@peterhaochen47
Copy link
Member

peterhaochen47 commented Jan 13, 2021

Hi @strehle, thanks for looking out for the test failure caused by the jasmine bump.
However, we think the approach proposed in this PR would not work because there is some significant obstacles to running docker inside of a docker image.

Instead, I think we can simply update the docker image where this jasmine test is run to have the correct versions of node & npm. For now, we are going to revert the PR that bumped jasmine (#1435) & and fix the test failure on that dependabot-initiated PR branch first.

@strehle
Copy link
Member Author

strehle commented Jan 14, 2021

@peterhaochen47 thank you for reverting the node update, so that pipeline is green again.
Yes I am check also regulary status there to prevent situation that we cannot release.

However the node update for jasmine is something I would do , but the pipeline shows, that it needs an update of used node/npm, see https://hush-house.pivotal.io/teams/cf-uaa/pipelines/uaa-acceptance-gcp/jobs/unit-tests-javascript/builds/390
╭───────────────────────────────────────╮
│ │
│ Update available 5.5.1 → 6.14.11 │
│ Run npm i -g npm to update │
│ │
╰───────────────────────────────────────╯

This version means outdated node, with node 12.x a npm version 6.x would be part, so either you update the local node version in the workers or you ensure that the used docker image cfidentity/uaa-singular is really used.
Test shows the pull of cfidentity/uaa-singular but later it is not used. If I manually pull and start cfidentity/uaa-singular then I see a recent node and npm version.

@peterhaochen47
Copy link
Member

peterhaochen47 commented Jan 15, 2021

Hi @strehle, thanks for identifying the source of the problem as the node/npm versions not matching what the new jasmine wants.
The reason why the CI job is not using the node/npm in the latest cfidentity/uaa-singular is because the cfidentity/uaa-singular image is tagged and the CI config specifies which tagged image it uses. We fixed it by updating that specification:

  type: docker-image
  source:
    repository: cfidentity/uaa-singular	  
    tag: 8.0.0	 # change to the newest => 92.77.0

Now the test is passing on #1493, which can now be merged.

@strehle
Copy link
Member Author

strehle commented Jan 15, 2021

Ok thanks for the info

@strehle strehle closed this Jan 15, 2021
@strehle strehle deleted the fix/jasmintest branch January 15, 2021 22:21
@cf-gitbot cf-gitbot added accepted Accepted the issue and removed delivered labels Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Accepted the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants