Skip to content
This repository has been archived by the owner on Nov 24, 2021. It is now read-only.

Fix npm security vulnerabilities #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix npm security vulnerabilities #41

wants to merge 1 commit into from

Conversation

autoboxer
Copy link

No description provided.

Copy link
Contributor

@jstockwin jstockwin left a comment

Choose a reason for hiding this comment

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

So I guess we should really have some tests on this repo...

There are a few things in the changelog that will cause issues here. See https://github.com/nightwatchjs/nightwatch/wiki/Migrating-to-Nightwatch-1.0.

In particular, I can see that we use e.g. use_ssl in our nightwatch.json, which has been deprecated in favour of using webdriver.ssl.

I've not contributed to KS for a long time now, but had a fair amount of input to this repo being created.

Do you still use this for KS testing? If so, you can do some kind of npm link to make the keystone tests use your local version and check they still run. I'd expect some kind of error due to the above deprecation. That should let you know if it's working or not...

@jstockwin
Copy link
Contributor

Also note this closes #39

@autoboxer
Copy link
Author

@jstockwin, I'm glad you pointed that out and didn't merge the PR. Installing locally and linking from Keystone is producing another error I can't seem to figure out, but is blocking all tests from running:

Cannot assign to read only property 'name' of function 'function HomeScreenGroup (config) {
        var tabs = {};
        config.tabs.forEach(function (tab) {
                if (tab.listName) {...<omitted>...
  }'

Most likely Keystone will eventually move to Cypress for testing, however, I'm going to spend some time to see if I can sort out an upgrade path here to help remove the last of the security vulnerabilities from Keystone.

@jstockwin
Copy link
Contributor

@autoboxer I haven't touched this in so long I wouldn't be able to offer any suggestions about that error, I'm afraid. Agree that removing the vulnerabilities is a great idea - from the changelog it seems like the changes should be trivial. Are the tests still passing in master somehow? I guess the first step is to get those working locally.

Fair enough regarding moving to Cypress, and from what I've read elsewhere it looks as though you're trying to move back to a single repo. My only comment on that would be to tell you why this one was created:

The idea was this was a way to expose the page objects for the admin ui used in the tests. This allows people with apps built on top of Keystone to simply include this repo in their dev requirements, and then they are able to use our page objects to interact with the admin UI etc (e.g. so they can easily log in and add a blog post, and then check it displays correctly on their frontend). This was quite a nice feature in our opinion at the time, but I don't know how widely it was used, or if you'd be able to keep something similar.

Happy with whatever upgrade path you guys choose - I don't have the time at the moment to be particularly involved, but I can probably answer any high-level questions about what's going on in this repo if that's helpful.

Good luck, and ping me if you need anything!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants