Add test fixtures by SergioSim · Pull Request #3 · openfun/ralph

SergioSim · 2020-05-19T15:05:45Z

Purpose

A event test fixture factory could help to test the function of the GELFParser

Proposal

Implementation of the event test fixture factory

jmaupetit

This is awesome!

SergioSim · 2020-07-06T16:37:44Z

    with pytest.raises(ValidationError):
        base_events(1, ip="  ")
-    with pytest.raises(ValidationError):
-        base_events(1, ip="051.255.9.27")


As the regex ip validation was removed this test does not raise an exception and silently corrects the ip from "051.255.9.27" to "51.255.9.27".
This causes the "event" to change after deserialisation/serialisation.
If this is not a problem it's good for me - removing this test

It's not a problem to me.

jmaupetit · 2020-07-07T08:23:22Z

+            raise ValidationError("Event should contain a parsable JSON string")
        keys = set(event.keys())
-        if len(required_keys.intersection(keys)) != 3:
+        if keys != required_keys:


I think this is not what we want, but rather:

Suggested change

if keys != required_keys:

if len(required_keys.intersection(keys)) != len(required_keys):

WDYT?

My concern here was that we might silently accept events having the required keys but also other keys that will be ignored in this case.
If this is not a problem, it's good for me, i will change that.

From your experience, does this occurs?

If I understood it right, the event field is created by the client then send to the django back-end and in the back-end there is no validation (it just tries to parse the json if it's a string)
I suggest the event field can't be trusted as it might contain any values.

Ok, so feel free to be more stringent then.

jmaupetit · 2020-07-07T08:26:01Z

+            )
+
+    @staticmethod
+    def parse_json_check_keys(data, required_keys):


Suggested change

def parse_json_check_keys(data, required_keys):

def validate_event_keys(data, required_keys):

jmaupetit · 2020-07-07T08:26:40Z

+    @staticmethod
+    def parse_json_check_keys(data, required_keys):
+        """Parse JSON string and check that it contains the keys in the
+        `keys_array` and no other keys


Good catch, thanks - required_keys is an set

jmaupetit · 2020-09-21T12:53:55Z

+    "textbook.pdf.searchcasesensitivity.toggled",
+]
+
+BROWSER_EVENT_VALID_AMAUNT = [


The same typo occurs in many lines:

Suggested change

BROWSER_EVENT_VALID_AMAUNT = [

BROWSER_EVENT_VALID_AMOUNT = [

Also look for amaunt: s/amaunt/amount/g

Good catch Julien, I made the mistake in my notes too, will change that

jmaupetit · 2020-09-21T13:02:54Z

+        if event["amaunt"] not in BROWSER_EVENT_VALID_AMAUNT:
+            raise ValidationError(
+                f"amaunt should be one of {BROWSER_EVENT_VALID_AMAUNT}"
+            )


IMO this is not a good idea to have a single BrowserEventSchema with x schema validation methods. This is not efficient at all! I would prefer having 1 schema per event and a BaseBrowserEventSchema to inherit from.

Good Idea, I will give it a try, thanks

slandrault · 2020-09-28T07:17:41Z

I didn't see any circle-ci config file. Maybe we could add one to run the test suite (and use it as a base in the future for more checks / build)? What do you think @jmaupetit ? Or maybe in a separate PR to also include some linters like in others openfun projects?

SergioSim · 2020-09-28T07:39:01Z

I didn't see any circle-ci config file. Maybe we could add one to run the test suite (and use it as a base in the future for more checks / build)? What do you think @jmaupetit ? Or maybe in a separate PR to also include some linters like in others openfun projects?

Hello @slandrault ,
do you search this config file https://github.com/openfun/ralph/blob/master/.circleci/config.yml ?

slandrault · 2020-09-28T07:41:49Z

I didn't see any circle-ci config file. Maybe we could add one to run the test suite (and use it as a base in the future for more checks / build)? What do you think @jmaupetit ? Or maybe in a separate PR to also include some linters like in others openfun projects?

Hello @slandrault ,
do you search this config file https://github.com/openfun/ralph/blob/master/.circleci/config.yml ?

😅 Awesome, I don't know why I didn't see it, I was probably on the wrong branch. All good then, sorry about that!

slandrault · 2020-09-28T12:35:50Z

@@ -0,0 +1,307 @@
+"""
+Broser event factory definitions


s/Broser/Browser

Good catch, thanks, will update this

- add server schema to validate server events - add server event to xapi converter for the conversion - remove pandas dependency

SergioSim · 2021-02-10T23:18:13Z

Closing this pull request as it will be split into smaller pieces.
The first part is #59

SergioSim force-pushed the add-test-fixtures branch from 757aaa8 to 7b6a312 Compare May 19, 2020 15:12

SergioSim mentioned this pull request May 19, 2020

Add "event" test fixture factory #2

Closed

2 tasks

jmaupetit assigned SergioSim May 19, 2020

jmaupetit added feature FUN WIP and removed FUN labels May 19, 2020

jmaupetit self-requested a review May 19, 2020 19:18

SergioSim force-pushed the add-test-fixtures branch 4 times, most recently from 37d2271 to 2127a36 Compare May 20, 2020 15:31

SergioSim force-pushed the add-test-fixtures branch 2 times, most recently from cb90dbf to 11189c2 Compare May 28, 2020 11:57

SergioSim commented May 28, 2020

View reviewed changes

Comment thread tests/fixtures/event/proposal.py Outdated

SergioSim force-pushed the add-test-fixtures branch 3 times, most recently from f65af66 to 3ee5e11 Compare June 5, 2020 14:38

jmaupetit reviewed Jun 5, 2020

View reviewed changes

Comment thread tests/fixtures/event/proposal_marshmallow.py Outdated

SergioSim force-pushed the add-test-fixtures branch 4 times, most recently from a89d787 to 9b4ed23 Compare June 8, 2020 23:57

SergioSim commented Jun 11, 2020

View reviewed changes

Comment thread tests/fixtures/event/mixins.py Outdated

SergioSim force-pushed the add-test-fixtures branch from d03d14a to 6e876f7 Compare June 11, 2020 13:22

jmaupetit reviewed Jun 11, 2020

View reviewed changes

Comment thread src/ralph/schemas/edx/base.py Outdated

Comment thread src/ralph/schemas/edx/base.py Outdated

Comment thread src/ralph/schemas/edx/base.py Outdated

Comment thread tests/fixtures/event/mixins.py Outdated

Comment thread tests/fixtures/event/mixins.py Outdated

SergioSim force-pushed the add-test-fixtures branch 3 times, most recently from 6634331 to 499ccf3 Compare June 18, 2020 14:44

SergioSim commented Jul 6, 2020

View reviewed changes

SergioSim force-pushed the add-test-fixtures branch 3 times, most recently from c763f38 to cae290c Compare July 7, 2020 07:50

jmaupetit reviewed Jul 7, 2020

View reviewed changes

SergioSim force-pushed the add-test-fixtures branch 2 times, most recently from a370591 to 1c8a1e1 Compare July 7, 2020 10:24

jmaupetit reviewed Jul 7, 2020

View reviewed changes

Comment thread src/ralph/schemas/edx/browser.py Outdated

jmaupetit reviewed Sep 21, 2020

View reviewed changes

SergioSim force-pushed the add-test-fixtures branch from ec1b2eb to 6ca0282 Compare September 25, 2020 08:59

slandrault reviewed Sep 28, 2020

View reviewed changes

SergioSim force-pushed the add-test-fixtures branch 4 times, most recently from c69457d to 03bc861 Compare November 13, 2020 16:03

SergioSim force-pushed the add-test-fixtures branch from e3a7965 to f25b637 Compare January 14, 2021 21:11

SergioSim added 2 commits January 15, 2021 00:14

✨(converters) add edx server event xapi converter

43b9ace

- add server schema to validate server events - add server event to xapi converter for the conversion - remove pandas dependency

WIP ✨(converters) add edx browser, ORA and CAPA event xapi converters

2da63b7

SergioSim force-pushed the add-test-fixtures branch from f25b637 to 2da63b7 Compare January 14, 2021 21:22

WIP browser events

79279a2

SergioSim force-pushed the add-test-fixtures branch from fc44f41 to 79279a2 Compare January 18, 2021 21:06

SergioSim mentioned this pull request Feb 4, 2021

Ora2 Schemas and fixtures #6

Closed

10 tasks

SergioSim closed this Feb 10, 2021

MYilFun00 mentioned this pull request Jul 17, 2026

✨(api) add support for CORS #629

Open

4 tasks

	if keys != required_keys:
	if len(required_keys.intersection(keys)) != len(required_keys):

	def parse_json_check_keys(data, required_keys):
	def validate_event_keys(data, required_keys):

	BROWSER_EVENT_VALID_AMAUNT = [
	BROWSER_EVENT_VALID_AMOUNT = [

Uh oh!

Conversation

SergioSim commented May 19, 2020

Purpose

Proposal

Uh oh!

Uh oh!

jmaupetit left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slandrault commented Sep 28, 2020

Uh oh!

SergioSim commented Sep 28, 2020

Uh oh!

slandrault commented Sep 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioSim commented Feb 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants