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

Debugging and logging fixes, reworked unpacking action #2827

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

amartinz
Copy link
Member

@amartinz amartinz commented Sep 7, 2022

No description provided.

@codecov
Copy link

codecov bot commented Sep 7, 2022

Codecov Report

Merging #2827 (3dcd310) into master (efda0ce) will increase coverage by 0.02%.
The diff coverage is 80.00%.

❗ Current head 3dcd310 differs from pull request most recent head ebdfbc4. Consider uploading reports for the commit ebdfbc4 to get more accurate results

@@            Coverage Diff             @@
##           master    #2827      +/-   ##
==========================================
+ Coverage   82.58%   82.60%   +0.02%     
==========================================
  Files          29       29              
  Lines         982      989       +7     
==========================================
+ Hits          811      817       +6     
- Misses        171      172       +1     
Impacted Files Coverage Δ
src/lib/cli.js 100.00% <ø> (ø)
src/main.js 45.28% <0.00%> (ø)
src/core/plugins/core/plugin.js 98.93% <87.50%> (-1.07%) ⬇️
src/core/core.js 72.05% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

src/core/core.js Show resolved Hide resolved
group: "firmware",
files: [{ archive: "a.zip", dir: "a" }]
})); // TODO add assertions
// TODO add assertions
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should check exact params to the emits - imo we should test the functionality, not the implementation and I'd say the tests should not fail if we change the animation or wording on the messages - checking whether emit was called 3 times is enough I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

i followed the other tests. to actually test if it unpacks we would need to include a test file.
and if we just mock it all away, it kind of is useless as well?

not too common with tests, so will go the way you think is best.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should mock this unpack:

const { unpack } = require("../../helpers/asarLibs.js");

and then check if it was called with correct arguments

Copy link
Member Author

@amartinz amartinz Sep 7, 2022

Choose a reason for hiding this comment

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

Removed them and changed the todo message.

edit: we posted at almost the same time :D
will have a look at it, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

is it fine like this now?

src/lib/cli.js Show resolved Hide resolved
[14899:0907/145232.866716:ERROR:node_bindings.cc(244)] Error parsing Node.js cli flags
electron: [DEP0062]: `node --debug` and `node --debug-brk` are invalid. Please use `node --inspect` and `node --inspect-brk` instead.

Change-Id: I1930db5382d8efb8454e0c2136e19a6cf1d07293
Signed-off-by: Alexander Martinz <[email protected]>
Before:
  - debug: run killed with: {}
After:
  - debug: run killed with: TypeError: Cannot read properties of undefined (reading 'condition')

Change-Id: I640cb5dc0070a66537bd0188cb108be1ab2f7e12
Signed-off-by: Alexander Martinz <[email protected]>
@amartinz amartinz force-pushed the misc-fixes branch 5 times, most recently from eb3f5ab to ef47f00 Compare September 7, 2022 17:33
Unpacking actually expects the callback to always be provided,
either as second or third argument.

Change-Id: If81661d806ea58304c91d38a168535575ece8afd
Co-developed-by: Maciej Sopyło <[email protected]>
Signed-off-by: Alexander Martinz <[email protected]>
@amartinz amartinz merged commit 5463ab0 into ubports:master Sep 7, 2022
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.

2 participants