Regression tests configuration for f18 repository #861

CarolineConcatto · 2019-12-09T15:33:54Z

The configuration for the regression tests are in lit.* files. These tests rely on the
presence of llvm-lit and FileCheck, as long as F18 is not a part of the
monorepo it needs requires -DLLVM_EXTERNAL_LIT to be defined at cmake
configuration time to point to /bin/llvm-lit from an LLVM build.

This patch:

Changes CMakeLists.txt to:

Add the new folder with the regression tests configurations
Add lit configuration in test-lit/CMakeLists.txt

Adds the test-lit folder with the bare minimum configuration of
lit.cfg.py and lit.site.cfg.py.in to make a simple version lit test to work

Signed-off-by: Caroline Concatto caroline.concatto@arm.com

DavidTruby · 2019-12-09T16:25:54Z

README.md

+be defined at cmake command line:
+
+ LLVM_EXTERNAL_LIT=<LLVM_INSTALLATION_DIR>/bin/llvm-lit
+ LLVM_TOOLS_BINARY_DIR= <LLVM_INSTALLATION_DIR>/bin


nit: stray space here

Can the cmake file be changed to use LLVM_INSTALLATION_DIR as a cmake variable and then infer the values for LLVM_DIR, LLVM_EXTERNAL_LIT and LLVM_TOOLS_BINARY_DIR from the value of LLVM_INSTALLATION_DIR?

I believe that LLVM_EXTERNAL_LIT needs to belong in the cmake command. I've tried in the CMakeLists.txt and it did not worked very well. llvm-lit file was not being found properly

DavidTruby · 2019-12-09T16:30:01Z

test-lit/unit/lit.local.cfg

@@ -0,0 +1 @@
+config.suffixes = ['.f90', '.F90']


There are some tests in the test/ directory that use '.f' and '.F' as well, do those need to be included as suffixes here?

Probably yes, I only add the bare minimum. I believe as the tests are being added to the test-lit this file should reflect this change as well as the others.

Why not add them now and save some poor soul in the future from having to figure this out again?

documentation/OptionComparison.md has a list of file suffixes that other compilers accept, e.g. Cray accepts .f03 and .ftn. There's also a list in tools/f18.cc: "f", "F", "f18", "F18", "f90", "F90", "f95", "F95", "ff", "ff18", "ff90", and "ff95". The NAG compiler accept the 'ff' suffixes too.

sscalpone · 2019-12-09T16:54:08Z

This pull request looks fine.

Using the lit infrastructure is fine. However, when writing tests for compiler-emitted messages, my (perhaps outdated) understanding is that lit and its associated tools are unable to check for the precise location and number of error messages without, for example, hard-coding line numbers into the expected output.

Clang has a different tool for checking the content and placement of messages. Clang's system is documented here: https://clang.llvm.org/doxygen/classclang_1_1VerifyDiagnosticConsumer.html#details

Flang, like clang, has its own set of tools for precise checking of emitted messages, found in the shell scripts in the test subdirectories. (Python might be a better choice for portability, but that's a different topic.)

peterwaller-arm · 2019-12-10T15:19:17Z

Re @sscalpone

my (perhaps outdated) understanding is that lit and its associated tools are unable to check for the precise location and number of error messages without, for example, hard-coding line numbers into the expected output.

There is the [[#LINE + offset]] and [[#LINE - offset]] FileCheck syntax which allows you to avoid hard-coding line numbers in FileCheck tests:

http://llvm.org/docs/CommandGuide/FileCheck.html#filecheck-pseudo-numeric-variables

See, e.g: https://github.com/llvm/llvm-project/blob/396d18aeb6cb4409ed71ff4c331748ce1c530f33/clang/test/SemaCXX/warn-max-unsigned-zero.cpp#L13-L14, which uses both -verify and FileCheck.

CarolineConcatto · 2019-12-10T15:26:50Z

In the same line of thought I believe that DiagnosticConsumer would be implemented by Flang Driver.
But all the tests would be run by llvm-lit and we could use or FileCheck or --verify from diagnosis to check the regression tests.

sscalpone · 2019-12-10T16:55:25Z

@peterwaller-arm Thanks for the pointers. That looks pretty good to me.

@CarolineConcatto What is the advantage of --verify over FileCheck?

kiranchandramohan · 2019-12-11T11:36:48Z

test-lit/CMakeLists.txt

@@ -0,0 +1,32 @@
+# Copyright (c) 2018-2019, NVIDIA CORPORATION.  All rights reserved.


Why is this Nvidia copyright?
Also, do you want to use the new licensing here?

The problem of C&P

LLVM convention has removed the requirement for copyright. @gklimowicz is working on updating the f18 files with the new header.

It looks that in llvm CMakeLists.txt does not have a header like the tests.

kiranchandramohan · 2019-12-11T11:37:52Z

test-lit/unit/version_test.f90

+! Check that lit configuration works by checking the compiler version
+
+! RUN: f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
+! VERSION-DAG: f18 compiler


should this be a DAG check? Isn't DAG used when there are more than one check and the ordering is not significant?

Yes, it is wrong

kiranchandramohan · 2019-12-11T11:43:03Z

test-lit/lit.cfg.py

+# tools that might happen to be in the user's PATH.
+tool_dirs = [config.flang_tools_dir, config.llvm_tools_dir]
+
+tools = ['flang', 'f18', 'opt']


why is opt required?

I believe it is not required. ATM I am replacing by :
tools = ['%flang' ,'%f18']
But don't know if this is correct.

kiranchandramohan · 2019-12-11T11:54:55Z

README.md

+be defined at cmake command line:
+
+ LLVM_EXTERNAL_LIT=<LLVM_INSTALLATION_DIR>/bin/llvm-lit
+ LLVM_TOOLS_BINARY_DIR= <LLVM_INSTALLATION_DIR>/bin


Can the cmake file be changed to use LLVM_INSTALLATION_DIR as a cmake variable and then infer the values for LLVM_DIR, LLVM_EXTERNAL_LIT and LLVM_TOOLS_BINARY_DIR from the value of LLVM_INSTALLATION_DIR?

CarolineConcatto · 2019-12-11T14:58:07Z

Hi Steve as I understand they do different stuff.
--verify can get warnings emitted by the compiler. I don't know if FileCheck can.

CarolineConcatto · 2019-12-11T15:21:01Z

@kiranchandramohan , how much is ok to change the CMakeLists.txt

kiranchandramohan · 2019-12-11T16:23:42Z

@kiranchandramohan , how much is ok to change the CMakeLists.txt

I do not know what is acceptable here? @sscalpone can answer this I guess.

Can the cmake file be changed to use LLVM_INSTALLATION_DIR as a cmake variable and then infer the values for LLVM_DIR (existing), LLVM_EXTERNAL_LIT (new) and LLVM_TOOLS_BINARY_DIR (new) from the value of LLVM_INSTALLATION_DIR?

sscalpone · 2019-12-13T00:45:53Z

@kiranchandramohan , how much is ok to change the CMakeLists.txt

I do not know what is acceptable here? @sscalpone can answer this I guess.

Can the cmake file be changed to use LLVM_INSTALLATION_DIR as a cmake variable and then infer the values for LLVM_DIR (existing), LLVM_EXTERNAL_LIT (new) and LLVM_TOOLS_BINARY_DIR (new) from the value of LLVM_INSTALLATION_DIR?

When and why are the values of these macros different? What's the convention in the llvm makefiles?

tskeith · 2019-12-13T04:50:28Z

@kiranchandramohan , how much is ok to change the CMakeLists.txt

I do not know what is acceptable here? @sscalpone can answer this I guess.
Can the cmake file be changed to use LLVM_INSTALLATION_DIR as a cmake variable and then infer the values for LLVM_DIR (existing), LLVM_EXTERNAL_LIT (new) and LLVM_TOOLS_BINARY_DIR (new) from the value of LLVM_INSTALLATION_DIR?

When and why are the values of these macros different? What's the convention in the llvm makefiles?

It looks like LLVM_TOOLS_BINARY_DIR is already set by find_package(LLVM REQUIRED CONFIG) in the main CMakeLists.txt file.

tskeith · 2019-12-13T15:48:42Z

How do you run the lit tests?

CarolineConcatto · 2019-12-16T15:10:45Z

@tskeith I've updated the patch.
Now you need only to set:
DLLVM_INSTALL_UTILS=On
when building LLVM
and
-DLLVM_EXTERNAL_LIT=$LLVM_DIR/bin/llvm-lit
when building f18.

After that only by doing
make check-all
in the installation dir it should work

tskeith · 2019-12-16T15:48:42Z

@tskeith I've updated the patch.
Now you need only to set:
DLLVM_INSTALL_UTILS=On
when building LLVM
and
-DLLVM_EXTERNAL_LIT=$LLVM_DIR/bin/llvm-lit
when building f18.

I don't think you need LLVM_EXTERNAL_LIT on the cmake command if you set it like this:

set(LLVM_LIT_TOOLS_DIR$ "${LLVM_TOOLS_BINARY_DIR}/bin")
set(LLVM_EXTERNAL_LIT "${LLVM_TOOLS_BINARY_DIR}/llvm-lit")

CarolineConcatto · 2019-12-16T15:50:49Z

@tskeith I believe you are right!

RichBarton-Arm

Loads of comments, but aside from a few small errors (I think) we are well into the details now and I think this patch is almost ready.

I assume before submitting you will squash the commits together into one commit?

Also - typo in the commit message of this patch - prunning should be pruning.

RichBarton-Arm · 2019-12-16T15:37:12Z

CMakeLists.txt

@@ -58,6 +58,10 @@ message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION} in ${LLVM_DIR}")
 list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})

 include(AddLLVM)
+
+set(LLVM_TOOLS_BINARY ${LLVM_INSTALLATION_DIR}/bin)


I think this should be LLVM_TOOLS_BINARY_DIR.
Also - what is this used for? It seems like it is not used elsewhere in the PR.

Just in case LLVM_DIR has LLVM which has LLVM=$LLVM_PATH/lib/cmake/llvm
In README we should build f18 as:
LLVM=<LLVM_INSTALLATION_DIR>/lib/cmake/llvm cmake -DLLVM_DIR=$LLVM.
where is this LLVM_INSTALLATION_DIR defined?

We are using now LLVM variables

RichBarton-Arm · 2019-12-16T15:37:56Z

CMakeLists.txt

@@ -135,8 +140,8 @@ add_subdirectory(include/flang)
 add_subdirectory(lib)
 add_subdirectory(runtime)
 add_subdirectory(test)
-add_subdirectory(test-lit)


I liked it in the previous location, why the change?

I've changed because the CMakeLists.txt inside tools was needed for test-lit

README.md

RichBarton-Arm · 2019-12-16T15:42:51Z

README.md

@@ -81,7 +81,7 @@ where llvm is installed.

 ### LLVM dependency for lit Regression tests

- There are lit tests for the flang driver instance, these tests rely on the
+There are lit tests for the flang driver instance, these tests rely on the


This sentence is not future proof. I hope that in a the near-term future there will be more testing done with llvm-lit than just for the flang driver. We should re-word this to be something like "F18 has tests that use the lit framework, these tests rely on ...."

RichBarton-Arm · 2019-12-16T15:46:33Z

README.md

@@ -81,7 +81,7 @@ where llvm is installed.

 ### LLVM dependency for lit Regression tests


I think that with this patch anyone building F18 and running make check will encounter an error unless LLVM_EXTERNAL_LIT, etc. is specified in the make check line? If I have understood that correctly, then I think it would benefit the docs to have one example make line for F18 that includes both -DLLVM_DIR and -DLLVM_EXTERNAL_LIT. Perhaps put this at the bottom of this section?

You also need to add a note here about defining -DLLVM_INSTALL_UTILS=On to the part about building LLVM.

We do not need to set DLLVM_EXTERNAL_LIT if we do not want.

RichBarton-Arm · 2019-12-16T16:12:43Z

test-lit/lit.cfg.py


 llvm_config.use_default_substitutions()

 # excludes: A list of directories to exclude from the testsuite. The 'Inputs'
 # subdirectories contain auxiliary inputs for various tests in their parent
 # directories.
-config.excludes = ['Inputs', 'CMakeLists.txt', 'README.txt', 'LICENSE.txt', 'debuginfo-tests']
+config.excludes = ['CMakeLists.txt', 'README.txt', 'LICENSE.txt']


I think we should keep Inputs in this list. Sure we don't need it for any tests in the initial patch, but it is a well-understood feature of LLVM lit tests to have this directory so we should support it. Plus, your comment still talks about it despite it being removed from the list. Agree with removing debuginfo-tests.

RichBarton-Arm · 2019-12-16T16:18:39Z

test-lit/lit.site.cfg.py.in

@@ -3,8 +3,6 @@
 import sys

 config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"


Ah - now I think I understand the LLVM_TOOLS_BINARY_DIR need. I think you have used LLVM_TOOLS_BINARY_DIR and LLVM_TOOLS_DIR somewhat interchangeably in this patchset but, unless I am misunderstanding things, they mean to be the same thing. I think if you make it all line up this should work.

RichBarton-Arm · 2019-12-16T16:20:42Z

test-lit/unit/lit.local.cfg

@@ -1 +1,2 @@
-config.suffixes = ['.f90', '.F90']
+config.suffixes = [ '.f', '.F', '.f18', '.F18', '.f90', '.F90', '.f95', '.F95', '.ff', '.ff18', '.ff90', '.ff95']


I think it would be better to just set the global config.suffixes to all the fortran input file extensions F18 know about (earlier comment of mine) and then you don't need to set this here and the lit.local.cfg file can be removed.

This file was removed

RichBarton-Arm · 2019-12-16T16:21:41Z

test-lit/unit/version_test.f90

@@ -1,4 +1,4 @@
 ! Check that lit configuration works by checking the compiler version

-! RUN: f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
-! VERSION-DAG: f18 compiler
+! RUN: %f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s


I don't like 'unit' as a test subdirectory name. I think it should be driver, as that is what this test is testing.

This would still not catch lots of bad cases. All of the below version outputs would pass this test.

echo "f18 compiler (under development) foo" | FileCheck --check-prefix=VERSION version_test.f90
echo "foo f18 compiler (under development)" | FileCheck --check-prefix=VERSION version_test.f90
echo "f18 compiler (under development)

foo" | FileCheck --check-prefix=VERSION version_test.f90

RichBarton-Arm · 2019-12-16T16:26:55Z

test-lit/unit/version_test.f90

-! RUN: f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
-! VERSION-DAG: f18 compiler
+! RUN: %f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
+! VERSION: f18 compiler


When I run this I get

f18 compiler (under development)

Presumably this test is a demo of what the new framework can do rather than some placeholder for a real test. Therefore I think we may as well make this first lit test a proper test of functionality. -V is a great candidate and I expect that in the fullness of time the output might have some sort of sensible version number in it that we might check the format of with a regex in our test. For now it says this message, so regex is too much, but I think we should match the whole string in this test to be sure and also check there is no additional output after this line.

Put it another way, of we accidentally changed the message to something silly like "f18 compiler (foo bar wibble)" then this test wouldn't catch that for us would it?

RichBarton-Arm · 2019-12-18T14:25:56Z

CMakeLists.txt

@@ -59,8 +59,8 @@ list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})

 include(AddLLVM)

-set(LLVM_TOOLS_BINARY ${LLVM_INSTALLATION_DIR}/bin)
-set(LLVM_LIT_TOOLS_DIR$ ${LLVM_INSTALLATION_DIR}/bin)
+set(LLVM_LIT_TOOLS_DIR ${LLVM_INSTALATION_DIR} CACHE STRING "Command used to spawn lit")


One of these STRING values is wrong (both variables can't do the same thing surely?) and I assume it is this one.

RichBarton-Arm · 2019-12-18T14:54:38Z

test-lit/unit/version_test.f90

@@ -1,4 +1,4 @@
 ! Check that lit configuration works by checking the compiler version

-! RUN: f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
-! VERSION-DAG: f18 compiler
+! RUN: %f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s


This would still not catch lots of bad cases. All of the below version outputs would pass this test.

echo "f18 compiler (under development) foo" | FileCheck --check-prefix=VERSION version_test.f90
echo "foo f18 compiler (under development)" | FileCheck --check-prefix=VERSION version_test.f90
echo "f18 compiler (under development)

foo" | FileCheck --check-prefix=VERSION version_test.f90

RichBarton-Arm · 2020-01-03T18:07:14Z

CMakeLists.txt

@@ -60,7 +60,7 @@ list(APPEND CMAKE_MODULE_PATH ${LLVM_DIR})
 include(AddLLVM)

 set(LLVM_LIT_TOOLS_DIR ${LLVM_INSTALATION_DIR} CACHE STRING "Command used to spawn lit")
-set(LLVM_EXTERNAL_LIT ${LLVM_INTALATION_DIR}/llvm-lit CACHE STRING "Command used to spawn lit")
+set(LLVM_EXTERNAL_LIT ${LLVM_INTALATION_DIR}/llvm-lit)


Typos here - should be LLVM_INSTALLATION_DIR on this and line 62 (surprised everything still worked despite this!)

It still seems wrong to me. LLVM_LIT_TOOLS_DIR is surely a directory rather than a command so I think the cached string there is the one that needed changing. I can believe that LLVM_EXTERNAL_LIT pointing to llvm-lit executable is a "Command used to spawn lit". What does LLVM_LIT_TOOLS_DIR do? That should be the cached string.

It also seems a shame not to use it in line 63 to define LLVM_EXTERNAL_LIT.

Now our regression tests uses LLVM variables.
LLVM_EXTERNAL_LIT is in cache because LLVM does not cache this variable.

RichBarton-Arm · 2020-01-03T18:10:36Z

test-lit/driver/version_test.f90

@@ -1,5 +1,7 @@
 ! Check that lit configuration works by checking the compiler version

 ! RUN: %f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
+! VERSION: {{[[:space:]]}}


This test will still pass with the following incorrect output from f18 -V:

This is a bug

This is also a bug

f18 compiler (under development) This too

And this

The task to check if there is nothing else before the expected text is not trivial with FileCheck.
These discussions show how to implement a check of space with regression tests.
https://reviews.llvm.org/D28896 https://reviews.llvm.org/D29122
There is also the command CHECK-EMPTY but it cannot be used in the beginning of the checks
The task is also difficult to check the end of the file for nothing ever.

For the benefit of the review, Carol and I had a face-to-face conversation about this and agreed that as it is not trivial to make this test more robust using FileCheck and that our main aim with this simple test is just to prove the infrastructure she has added for FileCheck and lit works that it is not worth the trouble of making the test super robust at this point. We should revisit how best to make this test hardened at a later date. We may yet tweak the format of the version output and the new output might be more amenable to robust testing using FileCheck, so we might not hit this issue. We might also consider contributing further changes to FileCheck to make this test easier to write.

RichBarton-Arm · 2020-01-08T17:32:11Z

CMakeLists.txt

@@ -65,7 +69,7 @@ message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION} in ${LLVM_DIR}")
 # components (default is 'all') can be obtained with
 #
 #   llvm-config --libs --link-static [component ...]
-#
+ #


This stray space should be deleted.

RichBarton-Arm · 2020-01-08T17:34:53Z

test-lit/driver/version_test.f90

@@ -1,5 +1,7 @@
 ! Check that lit configuration works by checking the compiler version

 ! RUN: %f18 -V 2>&1 | FileCheck  -check-prefix=VERSION %s
+! VERSION: {{[[:space:]]}}


For the benefit of the review, Carol and I had a face-to-face conversation about this and agreed that as it is not trivial to make this test more robust using FileCheck and that our main aim with this simple test is just to prove the infrastructure she has added for FileCheck and lit works that it is not worth the trouble of making the test super robust at this point. We should revisit how best to make this test hardened at a later date. We may yet tweak the format of the version output and the new output might be more amenable to robust testing using FileCheck, so we might not hit this issue. We might also consider contributing further changes to FileCheck to make this test easier to write.

RichBarton-Arm · 2020-01-08T17:36:01Z

I think this is about ready to land. Remove the stray space and I will accept.

CarolineConcatto · 2020-01-13T15:53:43Z

@RichBarton-Arm extra space removed.

RichBarton-Arm

Thanks Carol. This all LGTM and I think we are ready to land.

@sscalpone can you merge it please?

sscalpone · 2020-01-14T07:38:59Z

@CarolineConcatto Please squash & update. Then I'll merge this PR. Thanks!

The configuration for the tests are in lit.* files. The lit tests rely on the presence of llvm-lit,FileCheck, not and count. When building LLVM add: -DLLVM_INSTALL_UTILS=On at the cmake command. LLVM_LIT is found by setting LLVM_EXTERNAL_LIT in f18 CMakeLists.txt. This patch: * Uses LLVM_EXTERNAL_LIT * Adds regression tests configurations * Adds a proof of concept regression test The regression test needs to have the Utils build in LLVM. This is done by adding: -DLLVM_INSTALL_UTILS=On to the LLVM build cmake. Signed-off-by: Caroline Concatto <caroline.concatto@arm.com>

CarolineConcatto · 2020-01-14T09:53:03Z

@sscalpone can you check if that is what you need in order to merge?

…iler/f18#861) The configuration for the tests are in lit.* files. The lit tests rely on the presence of llvm-lit,FileCheck, not and count. When building LLVM add: -DLLVM_INSTALL_UTILS=On at the cmake command. LLVM_LIT is found by setting LLVM_EXTERNAL_LIT in f18 CMakeLists.txt. This patch: * Uses LLVM_EXTERNAL_LIT * Adds regression tests configurations * Adds a proof of concept regression test The regression test needs to have the Utils build in LLVM. This is done by adding: -DLLVM_INSTALL_UTILS=On to the LLVM build cmake. Signed-off-by: Caroline Concatto <caroline.concatto@arm.com> Original-commit: flang-compiler/f18@a58c606 Reviewed-on: flang-compiler/f18#861

DavidTruby reviewed Dec 9, 2019

View reviewed changes

kiranchandramohan reviewed Dec 11, 2019

View reviewed changes

CarolineConcatto requested a review from RichBarton-Arm December 16, 2019 15:09

RichBarton-Arm suggested changes Dec 16, 2019

View reviewed changes

RichBarton-Arm reviewed Dec 18, 2019

View reviewed changes

CarolineConcatto mentioned this pull request Dec 20, 2019

Add new flang driver main() entrypoint #759

Closed

RichBarton-Arm reviewed Jan 3, 2020

View reviewed changes

RichBarton-Arm reviewed Jan 8, 2020

View reviewed changes

RichBarton-Arm approved these changes Jan 13, 2020

View reviewed changes

sscalpone approved these changes Jan 14, 2020

View reviewed changes

CarolineConcatto force-pushed the lit_configuration branch from ea37582 to 1049aeb Compare January 14, 2020 09:34

sscalpone merged commit a58c606 into flang-compiler:master Jan 14, 2020

		@@ -0,0 +1,32 @@
		# Copyright (c) 2018-2019, NVIDIA CORPORATION. All rights reserved.

		@@ -81,7 +81,7 @@ where llvm is installed.

		### LLVM dependency for lit Regression tests

		@@ -3,8 +3,6 @@
		import sys

		config.llvm_tools_dir = "@LLVM_TOOLS_DIR@"

		@@ -1 +1,2 @@
		config.suffixes = ['.f90', '.F90']
		config.suffixes = [ '.f', '.F', '.f18', '.F18', '.f90', '.F90', '.f95', '.F95', '.ff', '.ff18', '.ff90', '.ff95']

Regression tests configuration for f18 repository #861

Regression tests configuration for f18 repository #861

Conversation

CarolineConcatto commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sscalpone commented Dec 9, 2019

peterwaller-arm commented Dec 10, 2019

CarolineConcatto commented Dec 10, 2019

sscalpone commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarolineConcatto commented Dec 11, 2019

CarolineConcatto commented Dec 11, 2019

kiranchandramohan commented Dec 11, 2019

sscalpone commented Dec 13, 2019

tskeith commented Dec 13, 2019

tskeith commented Dec 13, 2019

CarolineConcatto commented Dec 16, 2019

tskeith commented Dec 16, 2019

CarolineConcatto commented Dec 16, 2019 • edited Loading

RichBarton-Arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichBarton-Arm commented Jan 8, 2020

CarolineConcatto commented Jan 13, 2020

RichBarton-Arm left a comment

Choose a reason for hiding this comment

sscalpone commented Jan 14, 2020

CarolineConcatto commented Jan 14, 2020

CarolineConcatto commented Dec 16, 2019 •

edited

Loading