Skip to content

A native application for running native unittests.#1224

Merged
chakrabot merged 1 commit intochakra-core:masterfrom
akroshg:nta
Jul 7, 2016
Merged

A native application for running native unittests.#1224
chakrabot merged 1 commit intochakra-core:masterfrom
akroshg:nta

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.

akroshg
akroshg reviewed Jun 30, 2016
View reviewed changes
bin/nativetests/JsRTApiTest.cpp

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

namespace JsRTApiTest
Copy link
Contributor Author

@akroshg akroshg Jun 30, 2016

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

jianchun commented Jun 30, 2016

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 •
edited
Loading

@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 •
edited
Loading

@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

jianchun commented Jun 30, 2016

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?

obastemur
obastemur reviewed Jun 30, 2016
View reviewed changes
bin/fileloadhelpers/FileLoadHelpers.cpp
//
// Read the entire content as a binary block.
//
fread((void*)contentsRaw, sizeof(char), lengthBytes, file);
Copy link
Collaborator

@obastemur obastemur Jun 30, 2016

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

@obastemur obastemur Jul 7, 2016

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

ianwjhalliday commented Jul 1, 2016

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.

@akroshg
A native application for running native unittests
013dc67 
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
@akroshg
[MERGE #1224 @akroshg] A native application for running native unitte…
0cce960 
…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.
@dilijev dilijev mentioned this pull request Jul 12, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@akroshg @jianchun @dilijev @ianwjhalliday @obastemur @msftclas @chakrabot