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

Client tests #225

Open
wants to merge 80 commits into
base: client-tests
Choose a base branch
from
Open

Conversation

MarhiievHE
Copy link
Member

Update client test

application/api/files.1/upload.js Outdated Show resolved Hide resolved
application/api/files.1/upload.js Outdated Show resolved Hide resolved
Copy link
Member

@tshemsedinov tshemsedinov left a comment

Choose a reason for hiding this comment

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

Is it subject to close #204 in favour of this PR?

@MarhiievHE
Copy link
Member Author

MarhiievHE commented May 5, 2023

@tshemsedinov Perhaps partially. Maybe it will be better if @leonpolak looks at whether everything is present, what he expected. I did not look at PR #204, but just corrected what interfered with the work of the test.
PS: I gave access to changes to my Fork.

application/lib/task1/start.js Outdated Show resolved Hide resolved
application/lib/task1/start.js Outdated Show resolved Hide resolved
test/client.js Outdated Show resolved Hide resolved
test/client.js Outdated
try {
const resources = await wsApi.example.resources();
test.strictEqual(resources?.total, null);
// console.log({resources});
Copy link
Member

Choose a reason for hiding this comment

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

Final version before landing should not contain commented console.log's

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, do I need to remove all commented code too?

Copy link
Member

Choose a reason for hiding this comment

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

Just before landing, or you can add comment // not for landing if you added something not related to this PR or commented something just to make this code works but if it should be subject to separate PR

@tshemsedinov
Copy link
Member

Looks like redis can't be accessed in docker: unhandledRejection: Error: connect ECONNREFUSED 127.0.0.1:6379
@MarhiievHE

@MarhiievHE
Copy link
Member Author

Looks like redis can't be accessed in docker: unhandledRejection: Error: connect ECONNREFUSED 127.0.0.1:6379 @MarhiievHE

You are right, I have forgotten to add optional support running API under a container with Redis. Ok, I will add a config for Redis.

@MarhiievHE
Copy link
Member Author

@tshemsedinov Do you have any other suggestions to improve or add some more tests?

@tshemsedinov
Copy link
Member

@tshemsedinov Do you have any other suggestions to improve or add some more tests?

Tests are still failing https://github.com/metarhia/Example/pull/225/checks

@MarhiievHE
Copy link
Member Author

MarhiievHE commented May 6, 2023

@tshemsedinov Do you have any other suggestions to improve or add some more tests?

Tests are still failing https://github.com/metarhia/Example/pull/225/checks

It's because the test.yml haven't any instance of Redis in the test environment.

@MarhiievHE MarhiievHE force-pushed the client-tests branch 3 times, most recently from 210cd9a to 075a1bb Compare May 8, 2023 21:26
@MarhiievHE MarhiievHE force-pushed the client-tests branch 5 times, most recently from b865f52 to 44364c7 Compare September 15, 2023 16:46
@MarhiievHE
Copy link
Member Author

@leonpolak @tshemsedinov
I rewrote the test to use a node test runner and fixed a subscription check. And I also corrected the GitHub Actions script. Now, all tests are completed correctly.

@MarhiievHE
Copy link
Member Author

MarhiievHE commented Sep 21, 2023

@leonpolak Did you talk about this error before? I caught it too now with node version 18.
image

19:59:09  W2   error   warning: MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 Symbol() listeners added to [MetaReadable]. Use emitter.setMaxListeners() to increase limit
  _addListener (node:events:587:17)
  MetaReadable.addListener (node:events:605:10)
  MetaReadable.once (node:events:649:8)
  eventTargetAgnosticAddListener (node:events:1004:15)
  node:events:966:5
  new Promise (<anonymous>)
  Function.once (node:events:949:10)
  MetaReadable.push (/node_modules/metacom/lib/streams.js:45:26)
  Server.binary (/node_modules/metacom/lib/server.js:300:18)

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