you latest PR received some comments, do you grasp the idea behind those comments?
Yaa Iāve already responded to those comments and solved all the issues which Laurent mentioned and submitted new commit after then itās been a 3 days I didnāt received any comments from Laurent regarding new commit looks like he is busy. I received one other comment regarding Build Test and Iāve also responded to that comment.
you have continued with @mic704b
but Iām a bit concerned about the following comments
-
https://github.com/laurent22/joplin/pull/2993#discussion_r404164387
instead of asking what to do have look successfully merged GSoC PRs and try to understand what makes a good unit test. -
https://github.com/laurent22/joplin/pull/2993#discussion_r404168347
why did you do this in the first place? Some explanation could be helpful as it leaves us with the impression that you may do it again, what is your lesson learned? -
https://github.com/laurent22/joplin/pull/2993#discussion_r404171783
it looks like that you havenāt read all important code, that puts you in bad position. Could you explain why you did not find these functions by yourself and what is your lesson learned?
Awaiting a ping pong game of Q&As until it is explained in details by Laurent and the others is unlikely to happen.
@bedwardly-down may you can correct or support my om some statements?
Iām getting the impression that those all show carelessness and rushing a bit without fully understanding how the code flows or even knowing to look for the parts that laurent is alluding to.
@kowalskidev, whatās going on? No need to rush if you donāt have to.
-
To make a build test for my code, I tried looking into existing build tests present in tests folder to understand how build tests are made, I looked for the tests something similar to my code trying to understand what type of build tests required for my code and as written in documentation we need to make new file for new features so I looked into
feature_ShowAllNotes.js
andfeature_TagList.js
and I figured out that build tests are made for the events called by GUI to make sure GUI is working correctly and also make sure settings are updated accordingly. -
So I made
feature_ThemeAutoDetect.js
file for my code and made tests for the events I created and new settings variables to make sure they are working properly but after the review it turned out that those tests are not needed so I'm little bit confused what to do now? and I've also looked into successful merged PRs but they are related to notes and folders but my case is totally different that's why I asked Laurent if any specific build tests required for my code.
- That's my mistake I didn't thought of reducing the code. I simply copied
theme
settings two times and changed name topreferredDarkTheme
&preferredLightTheme
further I didn't noticed thatoptions
property is repeating three time I should reduce to one. I'll take care of this next time...I learned a good lesson that the code should be clean and well structured means I should always try to avoid writing duplicated codes
- By referring to these functions did you mean
nativeTheme.shouldUseDarkColors
function? because this function I found by my self, I importednativeTheme
method from electron, It wasn't there before. Laurent is talking about the wrapper that I used to callnativeTheme.shouldUseDarkColors
function
-
shouldUseDarkColors
is a boolean function which returntrue
for dark system theme andfalse
for light system theme and I wrapped intosystemTheme()
function to get clear idea of we are getting values from detecting system native theme as simply readingshouldUseDarkColors
didn't gives idea that we are getting values from system native theme. -
Thank you so much for providing me your valuable suggestions and guidance, I'll take care of each of your points next time and I'll work hard to improve myself in upcoming days.
Yes Sir, I understand your concern. Iāll make sure it wonāt happen again.
thx for replying to all point.
I would prefer to be less apologizing etc. but more technical.
Less talking. If I read the answer I want to know what is going on instantly.
Only your first reply is technical but challenging to read as it is a whole paragraph, so I get easily lost between the lines
Iāve edited my answers now, I hope I explained what you are looking for.
The feature tests are for testing integrated behaviour, where we can simulate actions from the UI and check the response, usually by inspecting the resulting state. However for your PR, nearly all the behaviour is in the UI (in ElectronClient), so not much integration to test.
However unit tests are lower level and are used to test individual functions and classes in ReactNative code. You did make a change to one file there ( Setting.js
), try to figure out how it could tested at take from there.
may the following helps you:
joplin/CONTRIBUTING.md at master Ā· laurent22/joplin Ā· GitHub what gets you to https://github.com/laurent22/joplin/pull/2819#issuecomment-603502230
Besides this there is Topics tagged test
Thanks. That's a big help!
I reviewed my changes in Settings.js
, The changes Iāve made is addition of new variables namely themeAutoDetect
, preferredLightTheme
, preferredDarkTheme
and isNightMode
. Now to check this settings are working properly I created feature_ThemeAutoDetect.js
and make a test like:
it('should update Night Mode setting', asyncTest(async () => {
// SETUP: Set Night Mode value to true
testApp.dispatch({
type: 'SETTING_UPDATE_ONE',
key: 'isNightMode',
value: true,
});
await testApp.wait();
const state = testApp.store().getState();
expect(state.settings.isNightMode).toBe(true);
}));
This is how settings are tested right? Now talking about unit tests, When I was making build tests for the first time I found models_Setting.js
which I think it deals with all the changes in Setting.js
here I tried using Setting.setValue()
but it returns an error: Settings not initialized thatās why I created feature_ThemeAutoDetect.js
and added the test as shown above. Iām still thinking how it can be tested in other way apart from the tests I made.
That code looks clean. Something Iāve noticed from a small handful of PRs youāve submitted is that youāve been creating tests that are either redundant or unnecessary. I agree with Pack that one of the devs should look at this, but upon first glance, this looks better in terms of quality.
I looked for build tests related to other events and settings like Night Mode
button namely NOTELIST_VISIBILITY_TOGGLE
, SIDEBAR_VISIBILITY_TOGGLE
, noteListVisibility
etc.. just to name a few but I didn't found the specific build tests for these existing settings and events but as Laurent mentioned in the comment on my PR
that's probably already tested somewhere else in a more generic way. We don't need to check this for every single setting value.
So I think events and settings I created are already tested somewhere else along with the existing ones so probably I don't think we need to add new tests for my new settings and events.
Still, Let's wait for other mentors response.