Point stability, dependency updates & code quality

Development
1

Hello everyone!

The two major developments of the moment: Multi-user & Bluetooth presence in Gladys 4 having just been merged, I think it’s time to take a little break in terms of features and invest some time in stability, major updates of dependencies, and code quality.

I think it’s healthy to do this regularly to ensure that the project continues to be of the same quality as at the beginning!

Dependencies:

  • I was talking about it yesterday, it has become critical to review the frontend build process, still stuck under preact-cli@2.0.0: Mise à jour du build du front (preact-cli@2 actuellement)

  • On the DB side, we are currently on Sequelize 4, while Sequelize 6.5.0 has been released. I think there are many small optimizations/bugfixes that must have been released between these two versions, and it could do a lot of good to the project to update!

  • We are still on Node 12.xx, while Node 14 is the latest LTS version.

  • On the frontend side, we need to remove momentjs which is very heavy and deprecated! datejs that we use in the frontend does just as much work, and is lighter.

  • vis-data and vis-network take up a lot of space for not much, it’s really worth removing.

  • unistore, the state management library we use, has been updated and I think we really need to switch to it! Moreover, in general, I think we should stop using it for component data, and restrict ourselves to global variables (httpClient, user, that’s about it :p). We have a lot of small bugs that are due to the fact that we use this global store to store local component states. It was me who set it up, wrongly :slight_smile:

What do you think?

2

Is there confusion between frontend and backend, or are momentjs AND datejs used in parallel in the frontend?

Take a lot? What does that mean :sweat_smile:

For the rest, I completely agree, even if I can’t help with it yet.. Stability is super important and a rewrite is often necessary, you’ve shown that regularly in 4 versions of Gladys ..! Hat’s off :slight_smile:

3

momentjs and datejs are used on the frontend. Actually, we’re lucky, momentjs is only used in one place: the calendar :slight_smile: It’s my fault, I let it pass in the PR review, but clearly we need to get rid of moment.

take a *crazy place :slight_smile: Sorry I missed a word!

Top!

Indeed, I think that in any IT project, you should get used to dedicating at least 20% of the time to technical debt reduction. This allows you to have an up-to-date, maintainable, and clean project :slight_smile:

4

@AlexTrovato @cicoub13 @VonOx I don’t know if you saw my post, what do you think?

5

preact-cli@3

Yes, but I think major Front changes are difficult today because 0 tests. Same comment for unistore a bit below.

Is the idea to reset an empty project with version 3 and copy the Gladys code into this new updated project?

Sequelize 6.5.0

If we keep SQLite, go for this version. I can help

Node 14

I already see the MR and it should be quite easy

remove momentjs to replace with datejs

Yes, same: a lot of manual tests to do on the Front end

vis-data and vis-network

I don’t know what it’s used for, I think we can remove it.

unistore

What would you use instead for the Front store?

6

@pierre-gilles I indeed think it’s necessary, and that it’s something that needs to be done « day by day » and avoid waiting 6 months to do it.

As @cicoub13 says, there are no issues on the front end. Maybe we should spend some time on front-end tests before migrations/changes to ensure some non-regression campaigns, even if this will not replace the tests done by a human.
We already talked about it, and if I remember correctly, we mentioned https://www.cypress.io/

I think we will have to do it.

The best would be to divide the tasks, and nothing prevents us from starting the migrations without the Cypress tests.

Who takes what? (I take nodejs v14 :p)
Personally, I’m not very keen on the DB,
but I’m happy to start with Cypress, at least to lay the foundations and introduce tests on the « core » (you’ll have to be patient, it’s a discovery for me).

7

Go for it, I’ll watch you :sweat_smile:

I think I can do it

EDIT: The react component does not support dayjs :confused:
https://github.com/jquense/react-big-calendar#localization-and-date-formatting

8

Another related topic, we have dependencies with high level security vulnerabilities (12 front and 4 back). I know that most of the installations are not open on the web, but I think it’s better to update these dependencies.

I think it’s worth enabling DependaBot: Configuring Dependabot version updates - GitHub Docs

9

Lol you always find an excuse not to do it! :stuck_out_tongue:

10

10000% for!
At least automating the search for vulnerabilities in the code and setting it up to block on PRs is a good idea to ensure a minimum level of security.

11

I think we need the tests first

12

I agree for general maintenance, it’s day by day :slight_smile: (node version, minor/patch updates for dependencies, code quality).

After that, for major changes that bring nothing to the user/the project, I’m not necessarily for the dependency race.

We still have very limited means on the project, and frankly if you follow all the dependencies by updating as soon as a new one comes out (I’m not talking about security updates), well you don’t develop anymore.

Sequelize/tabler/etc… they have a very high breaking update rate compared to what we would be able to absorb on a daily basis. That’s why for these updates, we need to weigh the pros and cons and evaluate whether the pros are worth the time we could spend on other features « internal » to Gladys.. :slight_smile:

I agree!

I’m up for us to start with Cypress :slight_smile:

I think no one here has ever done Cypress, so in a way we should move forward together. If you want to look into doing a POC, and then we will all factor it together? I can also look into it on my side :slight_smile:

oh damn? Is it 100% sure? We could either propose a PR, or find a hack to make it work.. otherwise globalize or date-fns is less worse than moment

I saw it, that’s also the goal of this stability point :slight_smile:

I already have dependabot activated on the repo for the « notifications » part, after for the « automated PR » part I’m rather against.

On a project the size of Gladys, it creates dozens/hundreds of PRs per day, it completely fills the PRs and the CI, and in fact it’s just impossible to go as fast as him :stuck_out_tongue:

I’m for it :slight_smile: we need to put an npm audit in the CI and block only on « high/critical » vulnerabilities :slight_smile:

13

Okay, I have already started the POC for Cypress. I think I will create a few pages (login, dashboard, registration) to see the different aspects and integration with GitHub Actions, certainly.

I will try to keep a report for open questions and other points of difficulty encountered.

As soon as I have something small, I will share it so we can discuss it.

14

Good evening everyone, if some of you want to play or comment on the setup of cypress:

https://github.com/GladysAssistant/Gladys/pull/1086

The documentation (which evolves with the experimentation):

And now I’m playing with the GitHub actions to automate the thing, it’s rather good, but of course, it will lengthen the build time:

https://github.com/atrovato/Gladys/runs/2041711952?check_suite_focus=true

15

Hello @AlexTrovato! Great job, I’m putting my answers to your open questions here:

  1. File naming for tests:
  2. I think we shouldn’t name all our files index.js but rather try to split functional cases by file
  3. 1 file per case? (login_success.js / login_error.js)

One file per case seems excessive to me, don’t you think? On the server side, we do one test file per file in Gladys, generally one file per function. And then in the file, we test case by case, which seems cleaner to me, otherwise it will result in a lot of files?

  1. snake_case vs camelCase?

On the front end, it’s CamelCase with the first letter capitalized, I think we should keep the same rule for test files.

  1. Start the front end in development or production?

I don’t have experience with Cypress to know what the best practice is on this ^^

  1. Should we also start the server? Or mock the responses
  2. If we start the server, we can’t test the services/integrations
  3. If we mock the responses, more work, heavier maintenance

Good question. For me, the advantage of mocking would be for the services, but the disadvantage is that if an API route changes in the backend code, and the mocks are not updated, then the front end tests will pass when in fact they fail in real life. It’s less end-to-end.

Maybe a « hybrid » approach would be possible?

We mock everything that is services, and the Gladys core API (login, etc.) is not mocked so that we are sure that the critical routes (login, forgot password, signup) work well in E2E.

  1. How to detect that the front end is ready to automatically start the tests?

I don’t have experience with Cypress on this, we’ll have to investigate ^^

  1. How to properly manage the browser language?

There must be a way in cypress, right?

16

Hi,

I don’t really know Cypress, but if the goal is to do E2E testing, you should indeed leave the APIs behind because the principle is to test the assembly of the different components. If the goal is to do more unit testing on the front end, then it’s better to mock everything, even the core API.

I think you need both types of tests: E2E tests that indeed allow you to check that all the components work correctly together, and unit tests that allow you to know if a component has a problem or not.

For unit tests, it’s basically checking the behavior of the component according to all possible cases. This implies testing the props that you pass to it: if a component expects a number prop, you test with a number, then a string, then a null, etc., to verify that it handles all cases correctly. If there are events, you verify that they are called correctly (a click event should be called only once, etc.), etc. You also test state changes when necessary (basically, a button that would have a loading state, you need to verify that when the loading state is active, the button is in the correct representation, it has the loading class for example, etc.).

These are the most annoying tests to do. On small components, it’s quick, as soon as it’s a more complex component, it’s a mess ^^ It seems there are several possibilities to manage these tests. In the Preact documentation, it talks about enzyme or Preact testing library which allows you to do unit testing more simply, but well ^^ (Unit Testing with Enzyme – Preact Guide or Testing with Preact Testing Library – Preact Guide)

17

I took care of removing moment from the front end. I changed it to dayjs, but we also need to load the 2 plugins localizedFormat and localeData. Since the API is quite similar to moment, it works well. If we see problems appearing in the long term, I saw that we are using date-fns in ScheduledTrigger, which is also compatible with react-big-calendar, we can redirect to that.

https://github.com/GladysAssistant/Gladys/pull/1092

18
  1. Also start the server? Or mock the responses
  2. If we start the server, we cannot test the services/integrations
  3. If we mock the responses, more work, heavier maintenance

I agree with you to test the integration on the core and mock the services (at least for the success cases), we can easily mix the two.
I will complete the current PR to provide an example of both cases.
So, I think this allows testing in « production » mode to validate the target, in « e2e » mode, except for the services.

  1. How to detect that the front is ready to start the tests automatically?

Github actions can do that, there is the option in the PR CI.

  1. How to properly manage the browser language?

I think my question is rather: should we test the labels? It might be quite heavy. But to validate the error messages according to the cases, it can be useful.
Otherwise, it will be via execution variable/config.

19

Ok, good idea!

Nice :slight_smile:

It depends on the cases I would say. Indeed, for errors it could be good. Especially for certain « critical » pages (login, signup) where you really need to solidify.
Otherwise, no need, it might be redundant and heavy :slight_smile:

20

@bertrandda I just noticed that the Caldav service uses moment on the backend, it would be cool to change it here too :slight_smile: