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

[xtro] Split results by framework and add an HTML report #3093

Merged
merged 20 commits into from
Dec 15, 2017

Conversation

spouliot
Copy link
Contributor

No description provided.

@spouliot spouliot added the do-not-merge Do not merge this pull request label Dec 11, 2017
@spouliot
Copy link
Contributor Author

don't merge -> old data/comments files needs to be migrated.
Otherwise code is ready for review / feedback.

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build success

@monojenkins
Copy link
Collaborator

Build success

static class Log {
static Dictionary<string, List<string>> lists = new Dictionary<string, List<string>> ();

public static IList<string> On (string fx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting name :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's fluent ;-)

Console.WriteLine ("!unknown-native-enum! {0} bound", extra.Key);
var t = extra.Value;
if (!IsNative (t))
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why you went for a continue here.

I find:

if (IsNative (t)) {
    var framework = Helpers.GetFramework (t);
    Log.On (framework).Add ($"!unknown-native-enum! {extra.Key} bound");
}

simpler, more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

less nesting / horizontal scrolling ;-)
not that it makes a big difference in that specific case

var data = File.ReadAllLines (file);
// TODO: only process lines starting with '!'
foreach (var line in data)
list.Remove (line);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (line.StartsWith ("!")) and remove TODO?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but then I won't have anything to do ? ;-)

@@ -5,13 +5,14 @@ include $(TOP)/Make.config
# a 64bits mono is required because of the clang requirement
MONO ?= mono64 --debug

all-local:: run-ios run-osx run-watchos run-tvos classify
all-local::
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed if empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, makes make == make all

report-short:
JENKINS_SERVER_COOKIE=1 make report

all: build $(XIOS_PCH) $(XWATCHOS_PCH) $(XTVOS_PCH) $(XMAC_PCH)
Copy link
Contributor

@VincentDondain VincentDondain Dec 12, 2017

Choose a reason for hiding this comment

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

Shouldn't that be the first command in the Makefile so when we do a make it's equivalent to make all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make is already equal to make all - but not because of the order (rules are more complex)

@monojenkins
Copy link
Collaborator

Build success

@monojenkins
Copy link
Collaborator

Build success

@spouliot
Copy link
Contributor Author

@monojenkins
Copy link
Collaborator

Build failure

@spouliot spouliot added run-xtro-tests Run the xtro tests skip-all-tests Skip all the tests labels Dec 14, 2017
@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@monojenkins
Copy link
Collaborator

Build failure

@spouliot spouliot removed the run-xtro-tests Run the xtro tests label Dec 15, 2017
@spouliot
Copy link
Contributor Author

build

@monojenkins
Copy link
Collaborator

Build failure

@spouliot spouliot removed the skip-all-tests Skip all the tests label Dec 15, 2017
@spouliot
Copy link
Contributor Author

build

@monojenkins
Copy link
Collaborator

Build success

@spouliot
Copy link
Contributor Author

running skip-all-tess was the reason for most failures above
https://github.com/xamarin/maccore/issues/600

@spouliot spouliot removed the do-not-merge Do not merge this pull request label Dec 15, 2017
@spouliot spouliot changed the title [xtro] Split results by framework and add an HTML report (WIP) [xtro] Split results by framework and add an HTML report Dec 15, 2017
@monojenkins
Copy link
Collaborator

Build failure

@spouliot
Copy link
Contributor Author

known, random failure -> https://github.com/xamarin/maccore/issues/592

var count = ProcessFile (filename);
log.Write ("<td align='center'");
if (count < 1)
log.Write (" bgcolor='salmon'>-");
Copy link
Contributor

Choose a reason for hiding this comment

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

🐟

Copy link
Contributor Author

Choose a reason for hiding this comment

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

glad it did not went unnoticed ;-)
and now we can say there's something fishy about that PR :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

:iseewhatyoudidthere:

Copy link
Contributor

@chamons chamons left a comment

Choose a reason for hiding this comment

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

🎉

@spouliot spouliot merged commit 8d854a0 into xamarin:master Dec 15, 2017
@spouliot spouliot deleted the xtro-report-update branch December 15, 2017 19:08
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.

6 participants