search

mlpack / models

models built with mlpack
https://models.mlpack.org/docs
BSD 3-Clause "New" or "Revised" License
35 stars 40 forks source link

Porting tests from boost to catch2 #52

Closed Aakash-kaushik closed 3 years ago

Aakash-kaushik commented 3 years ago

This PR also has the commits from #47 (Fix Build)

I am a bit confused about the cmake part of it any help would be appreciated.

Aakash-kaushik commented 3 years ago

The same tests are failing as they were failing for boost

kartikdutt18 commented 3 years ago

Will take a look soon. Thanks for the PR.

Aakash-kaushik commented 3 years ago

Will take a look soon. Thanks for the PR.

Sure, welcome !

Aakash-kaushik commented 3 years ago

The builds are passing because the tests are actually not happening i need to look into the cmake file for that but some help would be really appreciated.

kartikdutt18 commented 3 years ago

Hey, could you merge the master in this branch (Should clean up some commits). Also I want to try something CMake and cereal as well. If it works, you could base your PR on it.

Aakash-kaushik commented 3 years ago

Hey, could you merge the master in this branch (Should clean up some commits). Also I want to try something CMake and cereal as well. If it works, you could base your PR on it.

Sure, I merged the master to my branch.

Aakash-kaushik commented 3 years ago

Hey @kartikdutt18, @zoq, @rcurtin first of all sorry for bothering you all in this but why aren't we using simple curl to download files when it's viable on all three platforms(Mac, linux, Windows 10 - 17063 and later) and why is it only being used for downloading files when the server is not mlpack.

I am talking about the DownloadFile function in utls.hpp

Aakash-kaushik commented 3 years ago

Theses tests are failing because they can't create folders when they do not exist. basically the folder named data here.

zoq commented 3 years ago

Theses tests are failing because they can't create folders when they do not exist. basically the folder named data here.

I guess in case of the failing test it could create the path, but not if there is a filename attached.

Aakash-kaushik commented 3 years ago

Theses tests are failing because they can't create folders when they do not exist. basically the folder named data here.

I guess in case of the failing test it could create the path, but not if there is a filename attached.

I already made some changes for this, you can take a look at the util.hpp file. That's why a lot more tests are passing now.

Aakash-kaushik commented 3 years ago

only 1 test to go, almost fixed everything else.

Aakash-kaushik commented 3 years ago

@zoq I think we can get this PR merged as the failing test is not related to the porting of tests from boost to catch2 and i can't figure out why suddenly there is one less column for the failing tests. @kartikdutt18 can you fill me up on this because i am not much aware on how this might be happening.

Aakash-kaushik commented 3 years ago

btw when you go to the build and see the step where tests are being run you will see that it now displays correct information about the build system and all of that the only thing i did for that was to include include(CTest) in the cmake file in the tests folder and it automatically generates the correct dartconfig, we can do this for mlpack too. just pretty things btw :D

Aakash-kaushik commented 3 years ago

I don't know why the windows tests aren't even starting.

zoq commented 3 years ago

btw when you go to the build and see the step where tests are being run you will see that it now displays correct information about the build system and all of that the only thing i did for that was to include include(CTest) in the cmake file in the tests folder and it automatically generates the correct dartconfig, we can do this for mlpack too. just pretty things btw :D

Nice, if you like to open a PR that would be great.

Aakash-kaushik commented 3 years ago

So the windows ctest error seems to be because it can't link or find the right dll file i tried a fix but that didn't work, do we have any other ideas to try ?

Aakash-kaushik commented 3 years ago

I am not sure if i can solve this, this is the issue where you have to copy the dll files to the same folder as you executable to make the ctest work.

Aakash-kaushik commented 3 years ago

Can you add ls after

https://github.com/mlpack/models/blob/1572442947f7834f30ea5d14237b8ef0444ddf96/.ci/windows-steps.yaml#L136

? Just to check whats in the directory.

I've done it, you should see the output in the next windows build

zoq commented 3 years ago

Can you add ls after https://github.com/mlpack/models/blob/1572442947f7834f30ea5d14237b8ef0444ddf96/.ci/windows-steps.yaml#L136

? Just to check whats in the directory.

I've done it, you should see the output in the next windows build

Great, can you also add it right before we enter build/tests.

Aakash-kaushik commented 3 years ago

Can you add ls after https://github.com/mlpack/models/blob/1572442947f7834f30ea5d14237b8ef0444ddf96/.ci/windows-steps.yaml#L136

? Just to check whats in the directory.

I've done it, you should see the output in the next windows build

Great, can you also add it right before we enter build/tests.

done.

Aakash-kaushik commented 3 years ago

image @zoq check this out :rocket:

zoq commented 3 years ago

@mlpack-jenkins test this please

zoq commented 3 years ago

Hm, I have to take a closer look into the style issue.

Aakash-kaushik commented 3 years ago

Hm, I have to take a closer look into the style issue.

Sure

Aakash-kaushik commented 3 years ago

LGTM!

I guess we can merge it then.

kartikdutt18 commented 3 years ago

Could you take in the latest pull and push again.

Aakash-kaushik commented 3 years ago

Could you take in the latest pull and push again.

Hey, I cannot understand what are you trying to convey. latest pull from where ?

kartikdutt18 commented 3 years ago

Could you force push by merging master into your branch so that tests run again or close and reopen the PR.

Aakash-kaushik commented 3 years ago

Could you force push by merging master into your branch so that tests run again or close and reopen the PR.

Pushed an empty commit, this should do it. btw the style checks just fail even when there is no issue.

kartikdutt18 commented 3 years ago

Will merge it, once build completes. Style checks can be fixed in another PR.

Aakash-kaushik commented 3 years ago

Will merge it, once build completes. Style checks can be fixed in another PR.

Passed again Also the style check has no issues actually just the build reports a failure. @zoq is looking into it.