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

Try to fix crafting tests on mingw builds #29957

Merged
merged 2 commits into from
Apr 28, 2019

Conversation

ifreund
Copy link
Contributor

@ifreund ifreund commented Apr 26, 2019

Summary

SUMMARY: None

Purpose of change

This is an attempt to debug/fix the crafting tests failing on the mingw builds for an unknown reason

Describe the solution

Add a REQUIRE for has_recipe() to see if this sheds any light on the issue.

 - This is an attempt to debug/fix the test failing on the mingw builds for an unknown reason
@ifreund ifreund added [C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling labels Apr 26, 2019
@ifreund
Copy link
Contributor Author

ifreund commented Apr 27, 2019

So I have no idea what's causing these tests to fail on the mingw builds. If anyone has any insight please let me know. I'll probably poke at it a bit more tomorrow.

@ifreund ifreund changed the title Try to fix crafting tests on mingw builds [WIP] Try to fix crafting tests on mingw builds Apr 27, 2019
@jbytheway
Copy link
Contributor

Based on the Travis test history looks like it was caused by #29949 (but I guess you already knew that).

I took a look at those changes but don't see how they could cause this failure.

If you're able to use docker then the docker image with the toolchain set up may be useful to you in debugging this.

 - This really shouldn't be needed, and the tests run fine without it
   with "normal" builds
@ifreund
Copy link
Contributor Author

ifreund commented Apr 28, 2019

Still don't know why these tests were failing, but adding a player::learn_recipe() call causes my local tests with a mingw build to pass. I really don't think this call should be necessary, but I couldn't figure out what was actually going on and this will patch over whatever problem is there and fix the tests.

@ifreund ifreund changed the title [WIP] Try to fix crafting tests on mingw builds Try to fix crafting tests on mingw builds Apr 28, 2019
@ZhilkinSerg ZhilkinSerg merged commit c731e22 into CleverRaven:master Apr 28, 2019
@kevingranade
Copy link
Member

I'm more confused as to why it isn't breaking elsewhere after that change.

@ifreund
Copy link
Contributor Author

ifreund commented Apr 28, 2019

I'm more confused as to why it isn't breaking elsewhere after that change.

Well, the crude_picklock recipe that's being tested is a difficulty 0 autolearn recipe, so it should be available to any new player.

@ifreund ifreund deleted the crafting-test branch April 28, 2019 09:32
@kevingranade
Copy link
Member

Hrm... do they need to proc a turn to learn it though?

@ZhilkinSerg
Copy link
Contributor

I don't think any recipes are somehow learned if you didn't call learn_recipe.

@jbytheway
Copy link
Contributor

But the recipe is somehow learned on non-Mingw platforms. So there's something funky here.

@ifreund
Copy link
Contributor Author

ifreund commented Apr 28, 2019

Should be learned when the character is created, if I'm reading the relevant code correctly:

// Learn recipes
for( const auto &e : recipe_dict ) {
const auto &r = e.second;
if( !knows_recipe( &r ) && has_recipe_requirements( r ) ) {
learn_recipe( &r );
}
}

It looks to me like the funkiness is happening on mingw, and the code is preforming as expected for the non-mingw platforms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Code: Tests Measurement, self-control, statistics, balancing. Crafting / Construction / Recipes Includes: Uncrafting / Disassembling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants