Skip to content
This repository was archived by the owner on Sep 17, 2025. It is now read-only.

System test#34

Merged
liyanhui1228 merged 23 commits intocensus-instrumentation:masterfrom
liyanhui1228:systest
Oct 24, 2017
Merged

System test#34
liyanhui1228 merged 23 commits intocensus-instrumentation:masterfrom
liyanhui1228:systest

Conversation

@liyanhui1228
Copy link
Copy Markdown
Contributor

No description provided.

@liyanhui1228 liyanhui1228 changed the title [WIP] System test System test Oct 16, 2017
@liyanhui1228 liyanhui1228 requested a review from duggelz October 18, 2017 22:43
@liyanhui1228
Copy link
Copy Markdown
Contributor Author

@duggelz I couldn't see your comment on the UI, the netcat-openbsd is for checking the connection to the flask/django app and wait for it to fully start and ready to response to requests.

Comment thread trace/nox.py Outdated


@nox.session
@nox.parametrize('python_version', ['2.7', '3.5'])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can we change this from 3.5 to 3.6?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread .circleci/config.yml Outdated
name: Install packages
command: |
apt-get update
apt-get install netcat-openbsd
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What's this for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

Comment thread trace/nox.py Outdated
'-s',
'tests/system/',
*session.posargs,
success_codes=range(0, 100)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What does this do? It seems like it ignores all error codes in the [1,99] range? Can we reduce this list, and document the expected error codes?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed. Will add documentation after several observations of the system tests failure(if any).

-in credentials.json.enc \
-out "$GOOGLE_APPLICATION_CREDENTIALS"
else
echo "No credentials. System tests will not run."
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you add exit 1 here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

Comment thread trace/tests/system/flask/main.py Outdated

app = flask.Flask(__name__)

# Enbale tracing, send traces to Stackdriver Trace
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Enbale -> Enable

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

'http://127.0.0.1:8080',
headers=self.headers_trace)

time.sleep(5)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We'll want to go back later and change the hardcoded sleep times to use retry or similar, but this is ok for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, will use retry in later PR.

self.assertEqual(
trace.get('spans')[0].get('parentSpanId'),
str(self.span_id))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you check the span attributes also?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added.

Comment thread trace/tests/system/flask/Dockerfile Outdated
ENV PATH /env/bin:$PATH
ADD . /app/
RUN pip install -r requirements.txt
EXPOSE 8000
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What's running on port 8000, and what's running on port 8080? Is there an inconsistency with some of the other code below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing out! Actually I'm going to remove this dockerfile since it's not used in the system test, just for running locally.

return process


class TestFlaskTrace(unittest.TestCase):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we could separate the "test harness" from the "start flask application", we could reduce some of the duplicated code between the django and flask tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, will fix in separate PR.


tracer.end_trace()

file = open(file_reporter.DEFAULT_FILENAME, 'r')
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If we later make tracing asynchronous (which we should), we'll have to remember to add some kind of wait or flush here, between closing the last span and opening the reporter file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do that.

@liyanhui1228 liyanhui1228 merged commit 99d3bbe into census-instrumentation:master Oct 24, 2017
@liyanhui1228 liyanhui1228 deleted the systest branch October 31, 2017 23:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants