[#465] [FEATURE] Amélioration de la gestion des tests adaptatifs (US-604). #465

jilljenn · 2017-07-12T09:32:14Z

Tests adaptatifs testés
Destruction de la table scenarios

jilljenn · 2017-07-12T13:24:43Z

Bon il ne reste plus qu'à :

Faire des méthodes plutôt que surcharger le prototype Set dans Assessment
Bouger la conversion des objets dans lib/infrastructure/adapters ? Est-ce toujours pertinent ?

coveralls · 2017-07-12T13:34:04Z

Coverage increased (+0.2%) to 95.126% when pulling f6a83f7 on improve-adaptive-tests into 5ff351e on dev.

coveralls · 2017-07-12T15:47:19Z

Coverage increased (+0.2%) to 95.126% when pulling f6a83f7 on improve-adaptive-tests into 5ff351e on dev.

coveralls · 2017-07-24T13:18:20Z

Coverage increased (+0.9%) to 95.136% when pulling 12572dd on improve-adaptive-tests into 78f57c8 on dev.

coveralls · 2017-07-24T13:19:39Z

Coverage increased (+0.3%) to 94.525% when pulling 12572dd on improve-adaptive-tests into 78f57c8 on dev.

jbuget · 2017-07-24T13:20:13Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

jbuget · 2017-07-24T13:20:54Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

jilljenn · 2017-07-25T07:57:36Z

J'ai eu une erreur chelou mais c'est quand même passé à la question suivante :

Erreur lors de l’enregistrement de la réponse : Error: Assertion Failed: 'answer:1' was saved to the server, but the response returned the new id '2'. The store cannot assign a new id to a record that already has an id.

Vous savez d'où ça vient ?

florianEnoh

Quelques remarques/suggestions que je te soumet à chaud. Je n'ai pas encore regardé les tests mais Good Job !

florianEnoh · 2017-07-26T12:52:24Z

api/lib/application/assessments/assessment-controller.js

-                const challengesLength = _.get(course, 'challenges.length', 0);
+                coursePix = course;
+                const challengePromises = coursePix.challenges.map(challengeId => challengeRepository.get(challengeId));
+                return Promise.all(challengePromises);


Hum ici tu on faire un return directement de la ligne au dessus.
Le résultat sera directement dispo pour le prochain then.

+1
Il est possible d'aplatir la chaine de promise de la même manière que le getNextChallenge() juste au dessus. Dispo si besoin d'un coup de main sur les promises.

florianEnoh · 2017-07-26T12:54:53Z

api/lib/application/assessments/assessment-controller.js


-                if (!course.isAdaptive) {
+                if (!coursePix.isAdaptive) {
                  return challengesLength > 0 && _.isEqual(answersLength, challengesLength);


La présence du return nous dispense du else.
On peut donc le virer :)

florianEnoh · 2017-07-26T13:15:03Z

api/lib/cat/answer.js

+  }
+
+  get binaryOutcome() {
+    if(this.result === 'ok') {


Ici on pourrait procéder de deux façons:
1- Comme le précédent commentaire, plus besoin du else.
2- Je propose ce petit shortcut (à adapter):

get binaryOutcome() { const isResultOk = this.result === 'ok'; return Math.floor(isResultOk); }

Ou simplement une ternaire, ce serait pas gênant ici.
return if(this.result === 'ok') ? 1 : 0;

florianEnoh · 2017-07-26T13:17:49Z

api/lib/cat/assessment.js

@@ -0,0 +1,119 @@
+Set.prototype.union = function(setB) {
+  const union = new Set(this);
+  for (const elem of setB) {


Je ne sais pas si j'ai bien compris l'intention mais je me dis qu'un coup de map ferait peut-être l'affaire.
genre:
return setB.map(union.add);
Tu me diras :)

J'aurais plutôt parié sur un reduce() du coup, mais je suis pas sûr.
Par contre, j'aurais bien aimé quelques tests sur les opérations (union, difference).

Je préfère laisser un for..each car c'est l'implem exacte donnée par la doc de Set.

Par ailleurs, ça permet d'avoir une implem très proche entre union et difference (dans le second cas, on peut aussi utiliser reduce, mais ce serait incompréhensible).

👍 pour les tests.

florianEnoh · 2017-07-26T13:49:51Z

api/lib/cat/assessment.js

+    const validated = new Set();
+    this.answers.forEach(answer => {
+      if (answer.result === 'ok') {
+        answer.challenge.skills.forEach(skill => {


Ici j'aurai pris ce raccourci: Avec Map ou ForeEach

return answer.challenge.skills.map((skill)=>{ return skill.getEasierWithin(this.course.tubes).forEach(validated.add); });

florianEnoh · 2017-07-26T14:30:10Z

api/lib/domain/services/assessment-service-utils.js

+  const assessment = AssessmentAdapter.getAdaptedAssessment(assessmentPix, answersPix, challengesPix);
+  if (assessment.nextChallenge) {
+    return assessment.nextChallenge.id;
+  } else {  // end of the test


Idem pour le return statement

Akhilian · 2017-07-31T09:02:20Z

api/lib/application/assessments/assessment-controller.js

-                const challengesLength = _.get(course, 'challenges.length', 0);
+                coursePix = course;
+                const challengePromises = coursePix.challenges.map(challengeId => challengeRepository.get(challengeId));
+                return Promise.all(challengePromises);


+1
Il est possible d'aplatir la chaine de promise de la même manière que le getNextChallenge() juste au dessus. Dispo si besoin d'un coup de main sur les promises.

Akhilian · 2017-07-31T09:07:05Z

api/lib/cat/answer.js

+  }
+
+  get binaryOutcome() {
+    if(this.result === 'ok') {


Ou simplement une ternaire, ce serait pas gênant ici.
return if(this.result === 'ok') ? 1 : 0;

Akhilian · 2017-07-31T09:10:27Z

api/lib/cat/assessment.js

@@ -0,0 +1,119 @@
+Set.prototype.union = function(setB) {
+  const union = new Set(this);
+  for (const elem of setB) {


J'aurais plutôt parié sur un reduce() du coup, mais je suis pas sûr.
Par contre, j'aurais bien aimé quelques tests sur les opérations (union, difference).

Akhilian · 2017-07-31T09:12:28Z

api/tests/unit/cat/course.spec.js

+  describe('#tubes', function() {
+    it('should exist', function() {
+      // given
+      /* const url1 = new Skill('url', 1);


Quelques commentaires en trop ;)

Akhilian · 2017-07-31T09:26:27Z

api/lib/domain/services/assessment-service.js

@@ -61,7 +69,7 @@ function selectNextChallengeId(course, currentChallengeId, assessment) {
    }

    if (course.isAdaptive) {
-      return resolve(_selectNextInAdaptiveMode(assessment));
+      return resolve(_selectNextInAdaptiveMode(assessment, course));


La fonction selectNextChallengeId pourrait être renommée en _selectNextChallengeId car elle n'est utilisée qu'ici et n'est pas publique.

Akhilian · 2017-07-31T09:28:21Z

api/lib/infrastructure/adapters/assessment-adapter.js

+const Answer = require('../../cat/answer');
+const Assessment = require('../../cat/assessment');
+
+function getAdaptedAssessment(assessmentPix, answersPix, challengesPix) {


Le code est un peu dense ici.
On pourrait ajouter des sauts de ligne et/ou découper certaines portions en fonctions privées ?

Par exemple, ligne 10 à 22 semble être un ensemble cohérent, une fonction privée qui explique ce que ça fait serait super cool pour la lecture.

Akhilian · 2017-07-31T09:33:39Z

api/tests/acceptance/application/assessment-controller-get-adaptive-correct_test.js

+    }];
+
+    beforeEach(function(done) {
+      knex('assessments').delete().then(() => {


On essaye de ne plus faire des imbrications de promises comme ça.
Tu peux t'inspirer de assessment-controller-get-solutions_test.js pour simplifier tout ça.

D'autant que si tu supprimes après chaque test, les phases de delete() sont inutiles dans le beforeEach() ;)

Akhilian · 2017-07-31T09:35:15Z

api/tests/acceptance/application/assessment-controller-get-adaptive-correct_test.js

+    it('should return the third challenge if the first answer is incorrect', function(done) {
+
+      const options = { method: 'GET', url: '/api/assessments/' + insertedAssessmentId + '/next/w_first_challenge' };
+      server.inject(options, (response) => {


De même, au lieu de inject() et de done, on essaye de faire via des promises.
Le fichier assessment-controller-get-solutions_test.js est un bon exemple.

L'avantage est de faire remonter la cause des erreurs bien plus explicitement.

le inject est la façon de faire des tests haut-niveau avec Hapi. difficile de s'en passer.
ok pour le done.

jbuget · 2017-08-10T10:06:28Z

ETQ équipe dev, JV l'algo adaptatitf en JS

coveralls · 2017-08-10T14:07:59Z

Coverage increased (+0.2%) to 94.537% when pulling f5b1417 on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-10T14:10:19Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

…iles into *_test.js

…ters

coveralls · 2017-08-10T15:52:43Z

Coverage increased (+0.2%) to 94.529% when pulling 4932c80 on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-10T15:54:06Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

coveralls · 2017-08-10T16:21:55Z

Coverage increased (+0.2%) to 94.523% when pulling ebe5fd1 on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-10T16:23:16Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

coveralls · 2017-08-11T07:19:29Z

Coverage increased (+0.2%) to 94.523% when pulling 8c23d0c on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-11T07:21:08Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

jbuget · 2017-08-11T09:04:23Z

en attente de la validation fonctionnelle par Benjamin

jilljenn · 2017-08-14T13:12:56Z

Pardon, il manque un dernier détail dont on avait déjà parlé avec @Akhilian et @hypernikao : le fait d'ajouter un test pour garantir qu'une évaluation ne dépasse jamais 20 questions. Je fais ça pour jeudi 17.

jilljenn · 2017-08-14T13:14:04Z

On pourrait faire ça dans une autre PR mais je pense que c'est suffisamment minimal (genre ajout d'un if et d'un test) pour faire partie de cette PR. Et ce sera relativement transparent pour la validation fonctionnelle de Benjamin, et on est couverts par les tests.

coveralls · 2017-08-17T15:14:21Z

Coverage increased (+0.2%) to 94.527% when pulling 174b0e9 on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-17T15:16:36Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

coveralls · 2017-08-21T13:39:38Z

Coverage increased (+0.2%) to 94.527% when pulling 7e84e24 on improve-adaptive-tests into 015c39c on dev.

jilljenn · 2017-08-23T11:45:59Z

Why coverage fail :'(

coveralls · 2017-08-23T13:00:19Z

Coverage increased (+0.2%) to 94.527% when pulling 7e84e24 on improve-adaptive-tests into 015c39c on dev.

jbuget · 2017-08-23T13:01:56Z

I've deployed this PR to http://improve-adaptive-tests.pix.beta.gouv.fr. Please check it out

…n (front) (US-618). (#492) * Add snapshot route and controller * Register snapshot route to general routes container * Add unit test to snapshot index route * Add snapshots migration * Add model snapshot and test * Remove unused test in snapshot model * Add snapshot repository * Add new tested error : InvalidAuthorizationHeaders * Add test for snapshot repository * Update snapshot migration with constraint on organisationId * Update snapshot model with constraint on organisationId * Add new method isOrganizationIdExist to check organization id existence * Add new error * Add a tested snapshot service to format snapshot raw * Add test on organization repository * Add test on snapshot-repository * Fix spelling error on feedback-panel * delete useless snapshot unit test * Add new error * Refacto snapshot controller * Add knex delete to snapshot repository * Fix bug OrganizationRepository return a promise * isOrganizationIdExist should return a promise with a boolean * test after refacto * Add "success notification" view to share-profile modal + lots of component refactor * Fix test error * Complete share-profile modal behaviour (from here, missing call to API and styles) * Add user id field to snapshot migration * Refacto snapshot repository with unit test * Add relation with users in snapshot model * Add deserialization attributes on snapshot controller * Refacto snapshot repository wirh logicless * Add logic to snapshot-service,return an snapshot id * Remove useless const * Add new serialization package * Add snapshot serializer * Little refacto snapsho controller * Refacto snapshot service to extract user id from serialized profile * Add acceptance test to snapshot controller * Add Snapshot model in Ember (and Mirage) * Improve acceptance test by making an assertion on the store/db call * [CLEANUP] Ajout d'un nouveau package jsonapi-serializer et refactoring de la sérialisation de l'authentificaiton (#490) * Add new package jsonapi-serializer module * Refacto authentication serializer * Refacto snapshot controller * Refacto snapshot service * Fix snapshot repository from code review * fix lint * [#465] [FEATURE] Amélioration de la gestion des tests adaptatifs (US-604). (#465) * Add CAT from pix-the-cat * Remove scenarios * Handle end of test case * Make eslint happy * Make eslint happy II * Add AssessmentAdapter * Take into account code review remarks * Add tests for Set #union and #difference methods + rename *.spec.js files into *_test.js * Flattenized (via #reduce instead of forEach) CAT assessment model getters * Remove useless params * Make function less compact * Refactor API acceptance tests * Add two tests, 20-question limit * Remove useless file * refacto snapshot service, delete comments and use fatarrow * Add snapshot route and controller * Register snapshot route to general routes container * Add unit test to snapshot index route * Add snapshots migration * Add model snapshot and test * Remove unused test in snapshot model * Add snapshot repository * Add new tested error : InvalidAuthorizationHeaders * Add test for snapshot repository * Update snapshot migration with constraint on organisationId * Update snapshot model with constraint on organisationId * Add new method isOrganizationIdExist to check organization id existence * Add new error * Add a tested snapshot service to format snapshot raw * Add test on organization repository * Add test on snapshot-repository * delete useless snapshot unit test * Add new error * Refacto snapshot controller * Add knex delete to snapshot repository * Fix bug OrganizationRepository return a promise * isOrganizationIdExist should return a promise with a boolean * test after refacto * Fix test error * Add user id field to snapshot migration * Refacto snapshot repository with unit test * Add relation with users in snapshot model * Add deserialization attributes on snapshot controller * Refacto snapshot repository wirh logicless * Add logic to snapshot-service,return an snapshot id * Remove useless const * Add new serialization package * Add snapshot serializer * Little refacto snapsho controller * Refacto snapshot service to extract user id from serialized profile * Add acceptance test to snapshot controller * Refacto snapshot controller * Refacto snapshot service * Fix snapshot repository from code review * fix lint * refacto snapshot service, delete comments and use fatarrow * Fix spelling error on feedback-panel * Add "success notification" view to share-profile modal + lots of component refactor * Complete share-profile modal behaviour (from here, missing call to API and styles) * Add Snapshot model in Ember (and Mirage) * Improve acceptance test by making an assertion on the store/db call * Fix bug when connecting back and front * Add CSS styles * Fix tests * Refactor pix-modal CSS styles (mobile part first) * Add tablet and desktop support * Fix snapshot model test by adding organization model relation * fix test bug by replacing modal close button class * Update modale interligne * Fix skipped unit tests * Reduce margin in desktop * Take into account code remarks * Add a unit test on compte#shareProfileSnapshot

jilljenn changed the title ~~Improve adaptive tests~~ [FEATURE] Improve adaptive tests Jul 12, 2017

jbuget changed the title ~~[FEATURE] Improve adaptive tests~~ [#465] [FEATURE] Amélioration de la gestion des tests adaptatifs (US-604). Jul 12, 2017

jbuget assigned jilljenn Jul 12, 2017

jilljenn added 6 commits July 24, 2017 22:11

Add CAT from pix-the-cat

d438985

Remove scenarios

23920e4

Handle end of test case

6f801e3

Make eslint happy

60cc395

Make eslint happy II

e614b1d

Add AssessmentAdapter

12572dd

jilljenn force-pushed the improve-adaptive-tests branch from 6b73e99 to 12572dd Compare July 24, 2017 13:11

florianEnoh reviewed Jul 26, 2017

View reviewed changes

Akhilian reviewed Jul 31, 2017

View reviewed changes

Akhilian added the [Tech] Changes required label Jul 31, 2017

Take into account code review remarks

f5b1417

jbuget added 2 commits August 10, 2017 16:45

Add tests for Set #union and #difference methods + rename *.spec.js f…

59920e3

…iles into *_test.js

Flattenized (via #reduce instead of forEach) CAT assessment model get…

4932c80

…ters

Remove useless params

d59b996

Refactor API acceptance tests

8c23d0c

jbuget added [Tech] Review asked and removed [Tech] Changes required labels Aug 11, 2017

jbuget added the Blocked label Aug 11, 2017

jbuget added [Tech] Changes validated and removed [Tech] Review asked labels Aug 11, 2017

Add two tests, 20-question limit

174b0e9

Remove useless file

7e84e24

jbuget merged commit 9b54583 into dev Aug 24, 2017

jbuget deleted the improve-adaptive-tests branch August 24, 2017 10:21

jbuget removed the Blocked label Aug 24, 2017

mackwic modified the milestones: v1.31.0, no-milestone Dec 5, 2017

[#465] [FEATURE] Amélioration de la gestion des tests adaptatifs (US-604). #465

[#465] [FEATURE] Amélioration de la gestion des tests adaptatifs (US-604). #465

Conversation

jilljenn commented Jul 12, 2017 • edited

jilljenn commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 12, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

jbuget commented Jul 24, 2017

jbuget commented Jul 24, 2017

jilljenn commented Jul 25, 2017 • edited

florianEnoh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbuget Aug 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbuget commented Aug 10, 2017

coveralls commented Aug 10, 2017

jbuget commented Aug 10, 2017

coveralls commented Aug 10, 2017

jbuget commented Aug 10, 2017

coveralls commented Aug 10, 2017

jbuget commented Aug 10, 2017

coveralls commented Aug 11, 2017

jbuget commented Aug 11, 2017

jbuget commented Aug 11, 2017

jilljenn commented Aug 14, 2017

jilljenn commented Aug 14, 2017

coveralls commented Aug 17, 2017

jbuget commented Aug 17, 2017

coveralls commented Aug 21, 2017

jilljenn commented Aug 23, 2017

coveralls commented Aug 23, 2017

jbuget commented Aug 23, 2017

jilljenn commented Jul 12, 2017 •

edited

jilljenn commented Jul 25, 2017 •

edited

jbuget Aug 10, 2017 •

edited