Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all tests over to Rely #59

Merged
merged 8 commits into from
Nov 20, 2019
Merged

Move all tests over to Rely #59

merged 8 commits into from
Nov 20, 2019

Conversation

ulrikstrid
Copy link
Collaborator

@ulrikstrid ulrikstrid commented Nov 19, 2019

This allows us to test on Windows and we also get nice jUnit reporting

@ulrikstrid ulrikstrid changed the title Start moving tests over to Rely Move all tests over to Rely Nov 20, 2019
@ulrikstrid ulrikstrid requested a review from wokalski November 20, 2019 07:53
# windows_408: { OCAML_VERSION: "4.08", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_406: { OCAML_VERSION: "4.06", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_407: { OCAML_VERSION: "4.07", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_408: { OCAML_VERSION: "4.08", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍 we can dev it on windows.

"@esy-ocaml/[email protected]@d41d8cd9"
],
"devDependencies": [ "[email protected]@d41d8cd9" ]
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much deps removed maybe we can go below < 400 MO of cache now.

package.json Outdated Show resolved Hide resolved

Alcotest.run(~argv=[|"--verbose --color"|], "Brisk", [("Core", core)]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is conserved ? "Brisk", [("Core", core)]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could nest the describe some more, do you want this @wokalski ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not relevant anymore. It's a relict of the distant past.

expectInt(
~label="The dispose should not have been run yet",
0,
effectDisposeCallCount^,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's on line 953, the diff is a bit hard to read here.

@@ -221,7 +222,8 @@ module EmptyComponentWithAlwaysEffect = {
};

module EmptyComponentWithOnMountEffect = {
let%component make = (~onEffect, ~onEffectDispose, ()) => {
let%component make =
(~onEffect: unit => unit, ~onEffectDispose: unit => unit, ()) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are this type annotation needed ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It complained about not being able to generalize a type, but this is only used in tests so it doesn't bleed anywhere.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird!

Copy link
Member

@wokalski wokalski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing. It's a lot of changes but I spotted only one mistake - there's a duplicate test.

# windows_408: { OCAML_VERSION: "4.08", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_406: { OCAML_VERSION: "4.06", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_407: { OCAML_VERSION: "4.07", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
windows_408: { OCAML_VERSION: "4.08", platform: Windows, vmImage: windows-latest, CACHE_FOLDER: $(Pipeline.Workspace)\cache },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥳

* outputs junit xml to the provided filepath, most CI solutions have
* integration with the junit xml format */
Default,
JUnit("./junit.xml"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤩

type mount = list(testMountEntry);

type testHostItem('a) =
| MountElement(mountElement): testHostItem(mount);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this wrapper is redundant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not new, TestHelpers.re is the old Assert.re but I removed some stuff at the end and added a new function.

type testHostItem('a) =
| MountElement(mountElement): testHostItem(mount);

type testState = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to move mountLog to testState. During insertNode child nodes should get a ref of mountLog to add updates to later. It'll make the tests less error prone. It's not a change for now though.

MountChild(root, div, 0),
]);
})
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

test("The effect should've been run", ({expect}) => {
state := state^ |> executeSideEffects;

expect.int(effectCallCount^).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

({expect}) => {
expect.int(effectDisposeCallCount^).toBe(1)
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

({expect}) => {
expect.int(effectDisposeCallCount^).toBe(1)
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

0,
effectDisposeCallCount^,
);
expect.int(effectCallCount^).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition @ulrikstrid 👏

MountChild(div, box("ImABox1"), 1),
]);
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@wokalski wokalski merged commit 013ee73 into master Nov 20, 2019
@wokalski wokalski deleted the move-to-rely-tests branch November 20, 2019 10:12
@wokalski wokalski mentioned this pull request Nov 20, 2019
@Et7f3 Et7f3 mentioned this pull request Nov 30, 2019
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants