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

test: fix ESP8266 Unit Tests #169

Merged
merged 5 commits into from Oct 28, 2019

Conversation

ciband
Copy link
Contributor

@ciband ciband commented Oct 25, 2019

Summary

This PR fixes the broken unit tests for esp8266. The theme of this PR is breaking up the unit tests to allow the tests to run on the esp8266 hardware without running out of RAM and blowing the stack.

The current status of the PR has all tests passing except to_array and to_json. These tests simply do not fit into RAM as currently implemented. I think this should be a different issue and PR.

Checklist

  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

Fixes #169
Depends on #172

@ghost
Copy link

ghost commented Oct 25, 2019

Thanks for submitting this pull request! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@ghost ghost added the Complexity: Undetermined Needs specialized, in-depth review. label Oct 25, 2019
@ciband
Copy link
Contributor Author

ciband commented Oct 25, 2019

CI failures for coverage and Windows look to be CI issues and out of scope for this PR.

The Windows failure looks like a bug in the script to call the unit test exe. Should this be written up as an Issue and worked separately?

@sleepdefic1t
Copy link
Contributor

Windows Issues addressed in #170
CodeClimate issues addressed in #171

This is a good PR, but a fairly sizable one.
I'll give this a proper testing in these next couple days, and we'll see if we can get those other two merged first 👍

Quick thought, What are your feelings on splitting off the ESP8266/PIO part to a separate PR?
Splitting up tests to fit on smaller devices and enabling ESP8266 support are easily two different things, I'm sure these changes will be useful for more than just the 8266 anyways.

Nice work as always regardless, @ciband
Thank you.

@ciband
Copy link
Contributor Author

ciband commented Oct 27, 2019

@sleepdefic1t

I agree that this will be helpful on other small RAM platforms. I certain can split it up but I’m not exactly sure where to draw the line. I’d prefer to keep it as is as I think the changes are all dependent on each other. That being said if you can give me some more details on what you want to split I can work that.

@sleepdefic1t
Copy link
Contributor

True, they’re kinda tied together..

But only in that the split up tests enables 8266 to run them successfully.

Splitting them up would, hypothetically, allow tests to pass on other devices too.

So how about this PR just splits the tests with the purpose of enabling smaller platforms to pass unit tests.
Basically, everything except the IoT/test_main.cpp and test/PIO files.

Those could be in a separate PR that enables 8266 now that the unit tests can support smaller architectures.

* Leveraged broken up unit tests to restore support for
running unit tests on ESP8266.
* Changed testing program to loop execute the unit tests for easier
debugging
Added extern directory to .gitignore to prevent working tree polution
@ciband
Copy link
Contributor Author

ciband commented Oct 27, 2019

Split out files as requested. They are in #172 and they must be merged before this PR.

@faustbrian faustbrian changed the title Fix ESP8266 Unit Tests test: fix ESP8266 Unit Tests Oct 28, 2019
@ghost ghost added the Status: Contributor Approved The pull request has been approved by a contributor. label Oct 28, 2019
@ghost
Copy link

ghost commented Oct 28, 2019

A contributor has approved this PR. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait.

Thank you for your contribution!

@faustbrian faustbrian merged commit d9a6c85 into ArkEcosystemArchive:develop Oct 28, 2019
@ghost
Copy link

ghost commented Oct 28, 2019

Your pull request has been merged but was not assigned a bounty tier. A maintainer will assign a bounty tier to this pull request in the next few days.

@faustbrian faustbrian added the Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. label Oct 28, 2019
@ghost
Copy link

ghost commented Oct 28, 2019

Your pull request has been merged and marked as tier 4. It will earn you $20 USD.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bounty: Tier 4 Awarded for small features, refactorings, improvements. This is valued at 20 USD. Complexity: Undetermined Needs specialized, in-depth review. Status: Contributor Approved The pull request has been approved by a contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants