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

Minor fixes: race condition, npm audit fixes, white space interfering with tests on Windows #23

Merged
merged 19 commits into from
Feb 28, 2020

Conversation

JakubNer
Copy link
Contributor

@JakubNer JakubNer commented Mar 29, 2019

[1] race condition constructing _httpServer/_httpsServer in ctor/init versus call to boot? Also inconsistent use of await on boot & stop in tests
[2] npm audit fixes (only dev dependencies)
[3] white space interfering with tests on Windows

@JakubNer JakubNer changed the title Minor fixes: [1] npm audit fixes [2] white space interfering with tests on Windows Minor fixes: race condition, npm audit fixes, white space interfering with tests on Windows Mar 29, 2019
https config wasn't respected in integration tests.

[1] respect either http or https opts being defined by themselves
[2] respect https opts if http is defined: previous change broke https
Copy link
Member

@silverbucket silverbucket left a comment

Choose a reason for hiding this comment

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

nice, lgtm, anyone else want to review? @skddc @galfert @lesion ?

@JakubNer
Copy link
Contributor Author

JakubNer commented Apr 15, 2019

Sorry guys if I'm making things confusing, but theses additional changes (4d3694a..6be492c) are pretty simple, confined to /example folder.

Note that 87f65f0 is a revert of inadvertent changes to .gitignore regarding /bin folder, so .gitignore is back to what it was before I got my hands on things.

4d3694a are some notes I accidentally checked in that are reverted with 77620b6. They shouldn't show up in the diff.

As for the rest of these commits, it's all in the /example folder. I made a youtube video:

https://youtu.be/c_0lU588kdk

@thornjad thornjad self-requested a review April 16, 2019 20:49
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

I think making boot and stop async should warrant a bump to 0.2.0.

I really like this solution for the race condition, I'm glad you caught it! And great additions to the example!

The lock file conflict should only need a merge master.

example/README.md Outdated Show resolved Hide resolved
example/README.md Outdated Show resolved Hide resolved
example/README.md Outdated Show resolved Hide resolved
example/README.md Outdated Show resolved Hide resolved
example/package.json Show resolved Hide resolved
lib/armadietto.js Outdated Show resolved Hide resolved
example/package.json Show resolved Hide resolved
[1] fix typos in README.md
[2] simplify promise in server init()
Copy link
Member

@thornjad thornjad left a comment

Choose a reason for hiding this comment

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

Awesome, looks great!

@raucao
Copy link
Member

raucao commented Feb 28, 2020

I think this should have been merged after the last comment, no?

/cc @lesion @thornjad @JakubNer @silverbucket

@silverbucket
Copy link
Member

@skddc Oh wow, totally forgot about this. I will merge, test, update deps and publish a new version.

@silverbucket silverbucket merged commit efcd690 into remotestorage:master Feb 28, 2020
AngeloR pushed a commit to AngeloR/armadietto that referenced this pull request May 20, 2021
… with tests on Windows (remotestorage#23)

* Fix webfinger tests checking XML to play
nice with whitespace

* update dev dependencies as per audit

* Fix race condition between ctro/init and boot

* fix 'returns resource metadata as XML' test

* don't hardcode port

* FIX: change b8aad6d broke https in integration:  passed unit testing but
https config wasn't respected in integration tests.

[1] respect either http or https opts being defined by themselves
[2] respect https opts if http is defined: previous change broke https

* Forgot to remove log

* fixups to make ./example work

* [oh] organizing thoughts

* [1] starting `npm run dev` opens debug port
[2] fix paths in example/index.html

* code and logging panes added to example

* Example application modified with the webfinger
and oauth endpoints.

* Example code finished

* update readme

* Revert "[oh] organizing thoughts"

This reverts commit 4d3694a.

* revert .gitignore back to previous version, not
ignoring all of bin/test-suite-storage

* fixup problems with hierarchical paths

* As per review comments:

[1] fix typos in README.md
[2] simplify promise in server init()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants