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

Fix merging documentations of modules and packages #1480

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

BarkingBad
Copy link
Contributor

@BarkingBad BarkingBad commented Sep 16, 2020

This PR will not build until #1473 will be merged into master

@@ -312,4 +312,66 @@ class LinkableContentTest : AbstractCoreTest() {
}

}

@Test
fun `Include module with description parted in two files`() {
Copy link
Member

Choose a reason for hiding this comment

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

This IMHO is the wrong kind of test. You should really write a unit test for everything that is unit testable.
I am not proposing to remove this test, but adding a proper unit test as well!

Copy link
Contributor

Choose a reason for hiding this comment

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

But with this engine we are doing mainly functional tests and this one does not look much different than the others

Copy link
Member

Choose a reason for hiding this comment

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

Yes the project is almost entirely relying on functional tests and this is not ideal! This particular component was designed to support unit tests.

Maybe @Kordyjan can give further directions on this issue :)

@@ -312,4 +312,66 @@ class LinkableContentTest : AbstractCoreTest() {
}

}

@Test
fun `Include module with description parted in two files`() {
Copy link
Contributor

Choose a reason for hiding this comment

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

But with this engine we are doing mainly functional tests and this one does not look much different than the others

Copy link
Contributor

@sellophane sellophane left a comment

Choose a reason for hiding this comment

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

Something here is not compiling...

@BarkingBad
Copy link
Contributor Author

BarkingBad commented Sep 29, 2020

Something here is not compiling...

Yeah, is should be rebased on top of #1473 as it uses its new API, so it is necessary to wait until former gets merged

@MarcinAman MarcinAman merged commit b278dcc into master Oct 8, 2020
@MarcinAman MarcinAman deleted the fix-merging-documnetation branch October 8, 2020 18:27
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.

None yet

5 participants