Test validator 37 #3

andypants888 · 2022-02-22T02:36:12Z

In this PR, the metaTest feature:

makes db calls to getInternalTest and getMetaTest
console.logs(the internal testing codes)
only returns false
does NOT evaluate metaTest YET.

oriooctopus · 2022-02-22T02:58:57Z

Can you include a video of this in action?

andypants888 · 2022-02-22T03:00:23Z

Can you include a video of this in action?

Sure. I will do that tomorrow.

oriooctopus · 2022-02-22T03:09:07Z

why is it called test validator 37?

oriooctopus · 2022-02-22T03:22:09Z

Off to a good start! Can you add a couple of unit tests? We don't have any yet, so you get to choose what we'll use. Perhaps testing-library? I've heard great things about it.... Message me before you settle on a final choice.

Actually, you know what, can you just create a separate ticket for that and we'll deal with it in the next PR? We want to avoid scope creep.

oriooctopus · 2022-02-22T02:59:28Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  beforeCreateMany(event) {
+    console.log("**** beforeCreateMany hook is activated! ****");
+    const eventData = event.params.data;
+    runMetaTest(eventData);
+  },


can you remove the many ones?

oriooctopus · 2022-02-22T02:59:43Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  beforeUpdate(event) {
+    console.log("**** beforeUpdate hook is activated! ****");
+    const eventData = event.params.data;
+    runMetaTest(eventData);


runMetaTests with an S

Also, the name should be more specific. If you're planning on them throwing an error inside, then you can say something like validateInternalTests. Validate typically implies something like that

Got it. I'm not 100% sure how I will implement it yet. I will change the name for the next PR once I decide? For now I can leave a comment about improving the name later.

Usually at the end, I try to rename the vars to make things easier to read/understand

oriooctopus · 2022-02-22T03:09:57Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+      select: ["id", "internalTest"],
+      where: {
+        id: {
+          $gte: getCurrentTestID(eventData),


what does $gte stand for? I am too lazy to look it up

It is "greater than or equal to" for filtering db.query in strapi. used with "where" keyword

$gte has been replaced with an array, to select the exact IDs needed

oriooctopus · 2022-02-22T03:10:34Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  let testIDs = [];
+  eventData.tests.forEach((test) => {
+    testIDs.push(test.id);
+  });


use map instead it's more elegant and easier to read

oriooctopus · 2022-02-22T03:11:01Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  eventData.tests.forEach((test) => {
+    testIDs.push(test.id);
+  });
+  const currentTestID = Math.min(...testIDs);


or actually use reduce that's even better

Then the entire function can be done in one line

although up to you, reduce will be fancy but maybe slightly less readable. Fancy can be fun though as long as it's not too tricky

I changed it to forEach to map. I'm going to leave it that way for now. I might have to tweak things before the feature is complete.

oriooctopus · 2022-02-22T03:14:17Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+async function getMetaTests(eventData) {
+  // Original Finding Internal Test
+  const MetaTests = await strapi.db.query("challenge.meta-test").findMany({
+    select: ["id", "caseCode", "expectedResult"],


can you change expectedResult to passes and make it a boolean? That is easier to configure

oriooctopus · 2022-02-22T03:17:02Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  const tests = await getInternalTests(eventData);
+  const metaTests = await getMetaTests(eventData);


can you use Promise.all here

oriooctopus · 2022-02-22T03:18:36Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  return internalTests;
+}
+
+function getCurrentTestID(eventData) {


I don't really understand this and getMetaTestId. Why are you getting the minimum value? I can't intuitively guess why

Also, it's a bad practice to have it receive the entire eventData. first of all, eventData is structurally different between beforeCreate and beforeUpdate, so you're running the risk of someone working on one of the lifecycles breaking the other. Second, it makes it harder to test this in unit tests because then you need to mock the entire massive object. In this case, just pass in the tests

I don't really understand this and getMetaTestId. Why are you getting the minimum value? I can't intuitively guess why

I will add a comment to make it more understandable. I ran into an problem where multiple tests/metatests were being returned out of order.

I needed a function Math.min(...test.IDs) to sort them by lowest ID --> highest

problem IDs (30, 29, 31)
after function (29, 30, 31)

Also, it's a bad practice to have it receive the entire eventData. first of all, eventData is structurally different between beforeCreate and beforeUpdate, so you're running the risk of someone working on one of the lifecycles breaking the other. Second, it makes it harder to test this in unit tests because then you need to mock the entire massive object. In this case, just pass in the tests

Okay I see what you mean here.

I didn't realize the event object would be changing. How do you know this?

How long have you been working with strapi?

Learning Strapi feels almost a little like learning react but for node...maybe this is why they call it a framework...

It's all patterns. You'll recognize many similarities with other frameworks. Strapi isn't very unique or anything

"I didn't realize the event object would be changing. How do you know this?" event objects are usually not standardized I think. Like even DOM events i would expect to change. In typescript you have a whoel bunch of different types for DOM events

andypants888 · 2022-02-25T03:11:38Z

why is it called test validator 37?

37 is the jira task number
Originally we called the feature: "a test-validator", renamed to "metaTest"

…est-validator-37

oriooctopus

Getting closer!

oriooctopus · 2022-02-28T23:43:11Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

@@ -0,0 +1,81 @@
+module.exports = {
+  beforeCreate(event) {
+    console.log("**** beforeCreate hook is activated! ****");


The console.log can be used for testing but shouldn't be merged. Make sure to remove it when you finish. If anything we would do more specific logging , but this is too generic

oriooctopus · 2022-02-28T23:43:32Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+    const [eventTests, eventMetaTests] = [
+      event.params.data.tests,
+      event.params.data.MetaTest,
+    ];


This doesn't make sense. Take a second look at it

You're right, but I am following the conventions of the Strapi Components. I could rename it to metaTests, but then it would look weird

on strapi. Which one do you think we should prefer?

oriooctopus · 2022-02-28T23:44:29Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+    .findMany({
+      select: ["id", "internalTest"],
+      where: {
+        id: getCurrentTestIDs(eventTests),


getCurrentINTERNALTestIds

Should I prefer Ids to IDs? I'm ok with either one but just not sure.

I read this SO response https://stackoverflow.com/questions/15526107/acronyms-in-camelcase/27172000

it's whatever. I think Ids is more common

I agree with the top answer, that's how I do it I guess

oriooctopus · 2022-02-28T23:45:00Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  let currentTestIDs = [];
+  eventTests.map((test) => {
+    currentTestIDs.push(test.id);
+  });


this defeats the point of map. You need to return inside of the map function. Take a look at map on MDN

function getCurrentTestIDs(eventTests) {
return eventTests.map(({id}) => id);
}

The eventTests array is an array of objects like
[{id: 10}, {id: 11}]
I am trying to create a new array of ids like this:
[10, 11]

Thanks for the heads up. I didn't realize .map() could be so flexible!

oriooctopus · 2022-02-28T23:45:16Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+}
+
+async function getMetaTests(eventMetaTests) {
+  const MetaTests = await strapi.db.query("challenge.meta-test").findMany({


should be camel case

oriooctopus · 2022-02-28T23:45:34Z

src/api/code-challenge/content-types/code-challenge/lifecycles.js

+  let metaIDs = [];
+  eventMetaTests.map((metaTest) => {
+    metaIDs.push(metaTest.id);
+  });


same as the above comment regarding map

andypants888 added 3 commits February 18, 2022 16:44

feature: Query Test & MetaTest internals

293e03e

feature: add findMany for multiple tests/metas

136b06a

feature: cleaned up lifecycle hooks

1ffb1a7

oriooctopus requested changes Feb 22, 2022

View reviewed changes

andypants888 added 2 commits February 28, 2022 13:07

Merge branch 'master' of https://github.com/oriooctopus/acc-v4 into t…

72f687e

…est-validator-37

feature: edits to pr#3 , 37-jira-task

3352422

oriooctopus requested changes Feb 28, 2022

View reviewed changes

feature: pr oriooctopus#3 edits round2

e55605a

oriooctopus merged commit d312b00 into oriooctopus:master Mar 1, 2022

andypants888 deleted the test-validator-37 branch March 1, 2022 19:41

		const tests = await getInternalTests(eventData);
		const metaTests = await getMetaTests(eventData);

Test validator 37 #3

Test validator 37 #3

Conversation

andypants888 commented Feb 22, 2022

oriooctopus commented Feb 22, 2022

andypants888 commented Feb 22, 2022

oriooctopus commented Feb 22, 2022

oriooctopus commented Feb 22, 2022

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

oriooctopus Feb 22, 2022 • edited Loading

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

andypants888 commented Feb 25, 2022

oriooctopus 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

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

oriooctopus Feb 22, 2022 •

edited

Loading