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

A native application for running native unittests. #1224

Merged
merged 1 commit into from
Jul 7, 2016

Conversation

akroshg
Copy link
Contributor

@akroshg akroshg commented Jun 30, 2016

This application is based upon the Catch framework (Open source). Most our internal tests are transformed from the TE to adopt the catch framework.


#pragma warning(disable:4100) // unreferenced formal parameter

namespace JsRTApiTest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are transformed from our full repo. So the intent of the tests have not been changed.

@akroshg
Copy link
Contributor Author

akroshg commented Jun 30, 2016

@digitalinfinity @dilijev @ianwjhalliday @EdMaurer have a look.

@jianchun
Copy link

Looks LoadScriptFile conflicts with changes in Linux branch

@akroshg
Copy link
Contributor Author

akroshg commented Jun 30, 2016

thanks @jianchun, I'll have a look at those. Also please have a look at the changes wrt to cross plat. I haven't done anything specific to enable that. That would be next stage.

@dilijev
Copy link
Contributor

dilijev commented Jun 30, 2016

@akroshg We will need to discuss whether it is appropriate to add the Microsoft copyright notice to bin/External/catch.hpp or to add an exclusion for that file or the entire directory.

If that code is licensed under a different license we might need to update LICENSE.txt and or thirdpartynotices.txt

/cc @EdMaurer @liminzhu on that decision.

For reference, full output of copyright check:

--------------
--- ERRORS ---
bin/External/catch.hpp ... does not contain a correct Microsoft copyright notice.

     /*
      *  Catch v1.5.6
      *  Generated: 2016-06-09 19:20:41.460328
      *  ----------------------------------------------------------

bin/nativetests/Scripts/splay.js ... does not contain a correct Microsoft copyright notice.

     ////////////////////////////////////////////////////////////////////////////////
     // base.js
     ////////////////////////////////////////////////////////////////////////////////


--------------

@akroshg
Copy link
Contributor Author

akroshg commented Jun 30, 2016

Sure @dilijev, I have initiated offline discussion for this.

@akroshg
Copy link
Contributor Author

akroshg commented Jun 30, 2016

Strange, *_release branch failures do not show up in the our buddy build system. @dilijev do you know if there are difference in between those?

@dilijev
Copy link
Contributor

dilijev commented Jun 30, 2016

@akroshg I think you mean the PreFAST warnings? The commands run in buddy builds are slightly different from the Jenkins scripts. We should probably add PreFAST analysis there. PreFAST analysis is only performed in release builds so that's why it only shows up in the release flavors.

You can see netci.groovy to see what commands are run in various builds so that you can reproduce the issues locally. Sent some more info about this offline.

To repro, you'll need to run:

call .\jenkins\buildone.cmd x64 release "/p:runcodeanalysis=true"  
powershell .\Build\scripts\check_prefast_error.ps1 . CodeAnalysis.err 

@jianchun
Copy link

Can I run it by cd bin/nativetests/Scripts and ../../../Build/VCBuild/bin/x86_debug/nativetests.exe? Hope it can run without any deployment steps. Other ideas: Take a command line arg -d <scripts dir>? Or, translate all scripts to char[] and embed in nativetests.exe at build time?

//
// Read the entire content as a binary block.
//
fread((void*)contentsRaw, sizeof(char), lengthBytes, file);
Copy link
Collaborator

Choose a reason for hiding this comment

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

the assumption fread has read entire content is wrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

@akroshg Is there a reason that you haven't change this one before merging?
RE: the assumption fread has read entire content is wrong

@ianwjhalliday
Copy link
Collaborator

This looks good to me at first glance. Some checkin gates have failed but overall I don't see anything I'd change about the approach taken.

This application is based upon the Catch framework (Open source). Most our internal tests are transformed from the TE to adopt the catch framework.
@chakrabot chakrabot merged commit 013dc67 into chakra-core:master Jul 7, 2016
chakrabot pushed a commit that referenced this pull request Jul 7, 2016
…sts.

Merge pull request #1224 from akroshg:nta

This application is based upon the Catch framework (Open source). Most our internal tests are transformed from the TE to adopt the catch framework.
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.

7 participants