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

Better unit tests #100

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Better unit tests #100

wants to merge 5 commits into from

Conversation

SamyPesse
Copy link
Member

@SamyPesse SamyPesse commented Aug 24, 2016

👷 WORK IN PROGRESS 🚧

This PR adds more (and better) unit tests to Nuts. Basically download and updates will be correctly unit tested to avoid breaking changes when trying to contribute to the repository.

This PR will also add standard HTTP status codes to responses, and fix #56.

I was hoping to move code to ES6 (cleaner code, especially for classes) and use Babel before npm releases, but I can't find a solution to make it work correctly while supporting the Heroku Deploy button (I'm open to ideas!).

I'll release a new version 🚀 as soon as all endpoints are correctly tested, and once I'm sure merging #95 did not break Nuts for GitBook and other devs.

XOXO 💕

@SamyPesse
Copy link
Member Author

I fixes #95, which was breaking the whole update system (stable will have been updated to unstable, etc).

@mzmousa can you look at the spec in test/update.js? And let me know if this is what you are expecting from update channels.

@SamyPesse
Copy link
Member Author

@jsloyer Since you are also interested in update channels, please look at the spec.

I want to be sure that we all agree on update channels:

  • /update/channel/beta should update the user to the latest beta or stable version
  • /update/channel/alpha should update the user to the latest alpha or stable version (maybe beta too?)
  • /update should update the user to the latest stable version (but accept unstable version if using one).

@mzmousa
Copy link
Contributor

mzmousa commented Aug 25, 2016

@SamyPesse This is the way I'm currently using it - I'm most just following how Squirrel-updates-server does it since it worked out so conveniently:
/update/channel/[alpha | beta] should update to the user to the latest [alpha | beta] version
/update/channel/stable should update the user to the latest stable version, but I think I agree with /update also updating to latest stable.

should update the user to the latest beta or stable version

What do you mean by this, specifically? I think I agree with having the stable version update as a fallback if something were to go wrong on the alpha or beta versions, but would we be able to have an error or something sent back in the JSON to specify why the fallback occured? :)

@alexstrat
Copy link

alexstrat commented Apr 10, 2017

I agree that /update should update the user to the latest stable version, and not any version — that's not the behavior implemented now on master.
Should I open a separate PR to fix this, or are we close to merge this one? Good job for the unit-test 👍 btw :)

@alexstrat
Copy link

alexstrat commented Apr 10, 2017

An other case I realize now: if you had a version 1.0.0-beta (ending-up in channel beta) and you release a version 1.0.0 (that ends up in channel stable then), shouldn't we expect that /update/channel/beta/osx/1.0.0-beta leads to 1.0.0?
It does not look to me that's the implemented behavior.

As a comparison, this is exactly the behavior implemented in electron-release-server: see here and here.

var nuts = require('../lib');

var config = {
repository: 'SamyPesse/nuts-testing',
Copy link

@alexstrat alexstrat Apr 11, 2017

Choose a reason for hiding this comment

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

Wouldn't it be more handy to implement and use a TestBackend (inherits from Backend) in which we could inject mock data? It would permit to test a large range of scenarios.

The GithubBackend looks thin and dumb enough so that test cases with TestBackend will cover a similar amount of logic. Moreover, some test cases (functional) can use the GithubBackend with the nuts-testing repo.


platform = platforms.detect(platform);

return that.versions.filter({
tag: '>='+tag,
platform: platform,
channel: channel
channel: channel,
stripChannel: true
Copy link

@alexstrat alexstrat Apr 11, 2017

Choose a reason for hiding this comment

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

I don't get why stripChannel should be striped.

Imagine we have a version 0.1.53-beta in releases (0.1.53 is not in release yet):
With stripChannel=true: /update/channel/beta/darwin_x64/0.1.53 leads to 0.1.53-beta => will install a lower version ⚠️
With stripChannel=false: /update/channel/beta/darwin_x64/0.1.53 leads to 204

Copy link

@alexstrat alexstrat Apr 11, 2017

Choose a reason for hiding this comment

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

I tried to remove the stripChannel but I ran into other problem: I was not able to update an user on a stable version to a beta version, because of how semver.satisfies handle the prerelease tag. Such a nightmare..

@julienma
Copy link

julienma commented May 8, 2017

Hey @SamyPesse, @alexstrat, I've tested this PR and it fixes the issue I had on /update (cf. #102 (comment) and #95 (comment)).

I'll use this PR as my main update server (as master does not work for me), but am of course wondering why it has not already been merged.
What's missing to get it stable (and merged)?

@biw biw mentioned this pull request Mar 17, 2021
@loprima-l
Copy link

Hi, I merged the project to a new repo to start maintain it, I would be glad if you can put your pull request here : https://github.com/loprima-l/nuts-2

rastiqdev added a commit to AnimeoTV/updateServer that referenced this pull request Jun 21, 2023
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.

5 participants